fix: Retain non-processable frames (#13447)

This is the proper fix for the same bug as #13446 as well as a revert of that PR.

I overestimated the effort for this, which is why we have two PRs in the first place.

src/sentry/lang/native/plugin.py

@@ -104,21 +104,16 @@ class NativeStacktraceProcessor(StacktraceProcessor):
         )
 
     def handles_frame(self, frame, stacktrace_info):
-        # XXX(markus): We cannot return false for any native frame here.
-        # Stacktrace processors have a (hard to fix) bug where all
-        # non-processable frames of a processable stacktrace are removed from
-        # an event.
-        #
-        # Before introducing Symbolicator (and calling it from enhancers) this
-        # never used to be a problem because even for mixed-platform
-        # stacktraces there was always at least one processor returning
-        # handles_frame() = True for each frame, so in the end no frame was
-        # discarded.
+        if not self.available:
+            return False
 
         platform = frame.get('platform') or self.data.get('platform')
         if platform not in self.supported_platforms:
             return False
 
+        if frame.get('data', {}).get('symbolicator_status') == 'symbolicated':
+            return False
+
         if 'instruction_addr' not in frame:
             return False
 
@@ -149,9 +144,6 @@ class NativeStacktraceProcessor(StacktraceProcessor):
             if pf.cache_value is not None:
                 continue
 
-            if pf.get('data', {}).get('symbolicator_status') == 'symbolicated':
-                continue
-
             obj = pf.data['obj']
             package = obj and obj.code_file
             # TODO(ja): This should check for iOS specifically.  Also

src/sentry/stacktraces/processing.py

@@ -340,12 +340,25 @@ def process_single_stacktrace(processing_task, stacktrace_info, processable_fram
     processed_frames = []
     all_errors = []
 
-    for processable_frame in processable_frames:
-        try:
-            rv = processable_frame.processor.process_frame(processable_frame, processing_task)
-        except Exception:
-            logger.exception('Failed to process frame')
-            rv = None
+    bare_frames = get_path(stacktrace_info.stacktrace, 'frames', filter=True, default=())
+    frame_count = len(bare_frames)
+    processable_frames = {frame.idx: frame for frame in processable_frames}
+
+    for i, bare_frame in enumerate(bare_frames):
+        idx = frame_count - i - 1
+        rv = None
+
+        if idx in processable_frames:
+            processable_frame = processable_frames[idx]
+            assert processable_frame.frame is bare_frame
+            try:
+                rv = processable_frame.processor.process_frame(
+                    processable_frame,
+                    processing_task
+                )
+            except Exception:
+                logger.exception('Failed to process frame')
+
         expand_processed, expand_raw, errors = rv or (None, None, None)
 
         if expand_processed is not None:
@@ -355,13 +368,13 @@ def process_single_stacktrace(processing_task, stacktrace_info, processable_fram
             processed_frames.extend(expand_raw)
             changed_processed = True
         else:
-            processed_frames.append(processable_frame.frame)
+            processed_frames.append(bare_frame)
 
         if expand_raw is not None:
             raw_frames.extend(expand_raw)
             changed_raw = True
         else:
-            raw_frames.append(processable_frame.frame)
+            raw_frames.append(bare_frame)
         all_errors.extend(errors or ())
 
     return (

tests/symbolicator/snapshots/SymbolicatorResolvingIntegrationTest/test_debug_id_resolving.pysnap

@@ -1,5 +1,5 @@
 ---
-created: '2019-05-29T11:21:36.394133Z'
+created: '2019-05-29T12:11:31.817415Z'
 creator: sentry
 source: tests/symbolicator/test_payload_full.py
 ---
@@ -25,16 +25,10 @@ exception:
   values:
   - raw_stacktrace:
       frames:
-      - abs_path: c:\projects\breakpad-tools\windows\crash\main.cpp
-        data:
-          symbolicator_status: symbolicated
-        filename: main.cpp
-        function: main
+      - function: <unknown>
         in_app: false
         instruction_addr: '0x2a2a3d'
-        lineno: 35
         package: C:\projects\breakpad-tools\windows\Release\crash.exe
-        symbol: main
     stacktrace:
       frames:
       - abs_path: c:\projects\breakpad-tools\windows\crash\main.cpp

tests/symbolicator/snapshots/SymbolicatorResolvingIntegrationTest/test_missing_debug_images.pysnap

@@ -0,0 +1,30 @@
+---
+created: '2019-05-29T12:45:17.433393Z'
+creator: sentry
+source: tests/symbolicator/test_payload_full.py
+---
+contexts: null
+debug_meta: null
+errors: null
+exception:
+  values:
+  - raw_stacktrace:
+      frames:
+      - function: hi
+        in_app: false
+      - function: unknown
+        in_app: false
+        instruction_addr: '0x100000fa0'
+    stacktrace:
+      frames:
+      - function: hi
+        in_app: false
+      - data:
+          symbolicator_status: unknown_image
+        function: unknown
+        in_app: false
+        instruction_addr: '0x100000fa0'
+    type: Fail
+    value: fail
+stacktrace: null
+threads: null

tests/symbolicator/snapshots/SymbolicatorResolvingIntegrationTest/test_missing_dsym.pysnap

@@ -1,5 +1,5 @@
 ---
-created: '2019-05-27T11:48:21.531011Z'
+created: '2019-05-29T12:45:20.193870Z'
 creator: sentry
 source: tests/symbolicator/test_payload_full.py
 ---
@@ -28,18 +28,22 @@ exception:
   values:
   - raw_stacktrace:
       frames:
+      - function: hi
+        in_app: false
       - data:
           symbolicator_status: missing
         function: unknown
-        in_app: false
+        in_app: true
         instruction_addr: '0x100000fa0'
         package: Foo.app/Contents/Foo
     stacktrace:
       frames:
+      - function: hi
+        in_app: false
       - data:
           symbolicator_status: missing
         function: unknown
-        in_app: false
+        in_app: true
         instruction_addr: '0x100000fa0'
         package: Foo.app/Contents/Foo
     type: Fail

tests/symbolicator/snapshots/SymbolicatorResolvingIntegrationTest/test_real_resolving.pysnap

@@ -1,5 +1,5 @@
 ---
-created: '2019-05-29T11:21:42.668708Z'
+created: '2019-05-29T12:45:22.803995Z'
 creator: sentry
 source: tests/symbolicator/test_payload_full.py
 ---
@@ -23,24 +23,22 @@ exception:
   values:
   - raw_stacktrace:
       frames:
-      - abs_path: /tmp/hello.c
-        data:
-          symbolicator_status: symbolicated
-        filename: hello.c
-        function: main
+      - function: hi
         in_app: false
+      - function: unknown
+        in_app: true
         instruction_addr: '0x100000fa0'
-        lineno: 1
         package: Foo.app/Contents/Foo
-        symbol: main
     stacktrace:
       frames:
+      - function: hi
+        in_app: false
       - abs_path: /tmp/hello.c
         data:
           symbolicator_status: symbolicated
         filename: hello.c
         function: main
-        in_app: false
+        in_app: true
         instruction_addr: '0x100000fa0'
         lineno: 1
         package: Foo.app/Contents/Foo

tests/symbolicator/snapshots/SymbolicatorUnrealIntegrationTest/test_unreal_crash_with_attachments.pysnap

@@ -1,5 +1,5 @@
 ---
-created: '2019-05-29T11:21:52.227050Z'
+created: '2019-05-29T12:11:45.277288Z'
 creator: sentry
 source: tests/symbolicator/test_unreal_full.py
 ---
@@ -105,35 +105,6 @@ exception:
       handled: false
       synthetic: true
       type: minidump
-    raw_stacktrace:
-      frames:
-      - data:
-          symbolicator_status: symbolicated
-        function: AActor::IsPendingKillPending
-        in_app: false
-        instruction_addr: '0x7ff754be3394'
-        package: \\Mac\Home\Desktop\WindowsNoEditor\YetAnother\Binaries\Win64\YetAnother.exe
-        raw_function: AActor::IsPendingKillPending()
-        symbol: AActor::IsPendingKillPending()
-        trust: context
-      registers:
-        r10: '0x7ffef000'
-        r11: '0x23d82c75ab0'
-        r12: '0x23d82c7d000'
-        r13: '0x3'
-        r14: '0x23df8f48bc0'
-        r15: '0x23df9a35d48'
-        r8: '0x8c3f2cd401'
-        r9: '0x7ffe03a9c86e'
-        rax: '0x64'
-        rbp: '0x8c3f2cd650'
-        rbx: '0x0'
-        rcx: '0x0'
-        rdi: '0x1'
-        rdx: '0x0'
-        rip: '0x7ff754be3394'
-        rsi: '0x0'
-        rsp: '0x8c3f2cd4c0'
     stacktrace:
       frames:
       - data:

tests/symbolicator/test_payload_full.py

@@ -40,10 +40,14 @@ REAL_RESOLVING_EVENT_DATA = {
             {
                 'stacktrace': {
                     "frames": [
+                        {
+                            "platform": "foobar",
+                            "function": "hi"
+                        },
                         {
                             "function": "unknown",
                             "instruction_addr": "0x0000000100000fa0"
-                        },
+                        }
                     ]
                 },
                 "type": "Fail",
@@ -168,6 +172,19 @@ class ResolvingIntegrationTestBase(object):
         assert event.data['culprit'] == 'unknown'
         insta_snapshot_stacktrace_data(self, event.data)
 
+    def test_missing_debug_images(self):
+        self.login_as(user=self.user)
+
+        payload = dict(project=self.project.id, **REAL_RESOLVING_EVENT_DATA)
+        del payload['debug_meta']
+
+        resp = self._postWithHeader(payload)
+        assert resp.status_code == 200
+
+        event = Event.objects.get()
+        assert event.data['culprit'] == 'unknown'
+        insta_snapshot_stacktrace_data(self, event.data)
+
 
 class SymbolicatorResolvingIntegrationTest(ResolvingIntegrationTestBase, TransactionTestCase):
     # For these tests to run, write `symbolicator.enabled: true` into your