Skip to content

Commit

Permalink
fix(sdk-crashes): Improve Cocoa crash detection (#52119)
Browse files Browse the repository at this point in the history
This PR adds a test case with frames from a real-world crash caused by
our MetricKit integration. All data was anonymized. While adding the
test case, a couple of problems surfaced that this PR addresses:
1. Correctly detect crashes from Swift files starting with SentryMX.
2. Correctly detect crashes from class extensions such as
NSDate(SentryExtras).
3. Ignore in_app when detecting an SDK crash, as customers can change
the in_app rules. Furthermore, if they use static linking for including
Sentry Cocoa, Cocoa SDK frames can be marked as in_app. Instead, the
algorithm only checks if frames are SDK frames or from system libraries.
  • Loading branch information
philipphofmann authored Jul 4, 2023
1 parent 856c373 commit e110e46
Show file tree
Hide file tree
Showing 4 changed files with 283 additions and 22 deletions.
25 changes: 23 additions & 2 deletions src/sentry/utils/sdk_crashes/cocoa_sdk_crash_detector.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ def is_sdk_crash(self, frames: Sequence[Mapping[str, Any]]) -> bool:
# The frames are ordered from caller to callee, or oldest to youngest.
# The last frame is the one creating the exception.
# Therefore, we must iterate in reverse order.
# In a first iteration of this algorithm, we checked for in_app frames, but
# customers can change the in_app configuration, so we can't rely on that.
# Furthermore, if they use static linking for including Sentry Cocoa, Cocoa SDK
# frames can be marked as in_app. Therefore, the algorithm only checks if frames
# are SDK frames or from system libraries.
for frame in reversed(frames):
# [SentrySDK crash] is a testing function causing a crash.
# Therefore, we don't want to mark it a as a SDK crash.
Expand All @@ -22,7 +27,7 @@ def is_sdk_crash(self, frames: Sequence[Mapping[str, Any]]) -> bool:
if self.is_sdk_frame(frame):
return True

if frame.get("in_app") is True:
if not self.is_system_library_frame(frame):
return False

return False
Expand All @@ -31,7 +36,12 @@ def is_sdk_frame(self, frame: Mapping[str, Any]) -> bool:

function = frame.get("function")
if function:
function_matchers = ["*sentrycrash*", "**[[]Sentry*"]
function_matchers = [
r"*sentrycrash*",
r"*\[Sentry*",
r"*(Sentry*)*", # Objective-C class extension categories
r"SentryMX*", # MetricKit Swift classes
]
for matcher in function_matchers:
if glob_match(function, matcher, ignorecase=True):
return True
Expand All @@ -44,3 +54,14 @@ def is_sdk_frame(self, frame: Mapping[str, Any]) -> bool:
return True

return False

def is_system_library_frame(self, frame: Mapping[str, Any]) -> bool:
system_library_paths = {"/System/Library/", "/usr/lib/"}

for field in self.fields_containing_paths:
for system_library_path in system_library_paths:
field_with_path = frame.get(field)
if field_with_path and field_with_path.startswith(system_library_path):
return True

return False
17 changes: 3 additions & 14 deletions src/sentry/utils/sdk_crashes/event_stripper.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,24 +153,12 @@ def _strip_frames(
We need to adapt this logic once we support other platforms.
"""

fields_containing_paths = {"package", "module", "abs_path"}

def is_system_library(frame: Mapping[str, Any]) -> bool:
system_library_paths = {"/System/Library/", "/usr/lib/system/"}

for field in fields_containing_paths:
for path in system_library_paths:
if frame.get(field, "").startswith(path):
return True

return False

def strip_frame(frame: MutableMapping[str, Any]) -> MutableMapping[str, Any]:
if sdk_crash_detector.is_sdk_frame(frame):
frame["in_app"] = True

# The path field usually contains the name of the application, which we can't keep.
for field in fields_containing_paths:
for field in sdk_crash_detector.fields_containing_paths:
if frame.get(field):
frame[field] = "Sentry.framework"
else:
Expand All @@ -181,5 +169,6 @@ def strip_frame(frame: MutableMapping[str, Any]) -> MutableMapping[str, Any]:
return [
strip_frame(frame)
for frame in frames
if sdk_crash_detector.is_sdk_frame(frame) or is_system_library(frame)
if sdk_crash_detector.is_sdk_frame(frame)
or sdk_crash_detector.is_system_library_frame(frame)
]
10 changes: 9 additions & 1 deletion src/sentry/utils/sdk_crashes/sdk_crash_detector.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
from abc import ABC, abstractmethod
from typing import Any, Mapping, Sequence
from typing import Any, Mapping, Sequence, Set


class SDKCrashDetector(ABC):
@property
def fields_containing_paths(self) -> Set[str]:
return {"package", "module", "abs_path"}

@abstractmethod
def is_sdk_crash(self, frames: Sequence[Mapping[str, Any]]) -> bool:
"""
Expand All @@ -15,3 +19,7 @@ def is_sdk_crash(self, frames: Sequence[Mapping[str, Any]]) -> bool:
@abstractmethod
def is_sdk_frame(self, frame: Mapping[str, Any]) -> bool:
raise NotImplementedError

@abstractmethod
def is_system_library_frame(self, frame: Mapping[str, Any]) -> bool:
raise NotImplementedError
Loading

0 comments on commit e110e46

Please sign in to comment.