-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(sdk-crashes): Ignore SentrySwizzleWrapper false positives (#105625) #105705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(sdk-crashes): Ignore SentrySwizzleWrapper false positives (#105625) #105705
Conversation
|
@philipphofmann I’ve opened this PR and would appreciate a review when you have a chance. Thanks! |
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for tackling this @calm329. I added a few comments.
tests/sentry/utils/sdk_crashes/test_sdk_crash_detection_cocoa.py
Outdated
Show resolved
Hide resolved
| # This SDK frame is BELOW SentrySwizzleWrapper (further from crash origin) | ||
| "function": "-[SentryHub captureEvent:]", | ||
| "package": "/private/var/containers/Bundle/Application/59E988EF-46DB-4C75-8E08-10C27DC3E90E/iOS-Swift.app/Frameworks/Sentry.framework/Sentry", | ||
| "in_app": False, | ||
| }, | ||
| { | ||
| "function": "-[UIApplication sendEvent:]", | ||
| "package": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore", | ||
| "in_app": False, | ||
| }, | ||
| { | ||
| "function": "__49-[SentrySwizzleWrapper swizzleSendAction:forKey:]_block_invoke_2", | ||
| "package": "/private/var/containers/Bundle/Application/59E988EF-46DB-4C75-8E08-10C27DC3E90E/iOS-Swift.app/Frameworks/Sentry.framework/Sentry", | ||
| "in_app": False, | ||
| }, | ||
| { | ||
| "function": "-[NSString substringWithRange:]", | ||
| "package": "/System/Library/Frameworks/Foundation.framework/Foundation", | ||
| "in_app": False, | ||
| }, | ||
| { | ||
| "function": "__exceptionPreprocess", | ||
| "package": "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation", | ||
| "in_app": False, | ||
| }, | ||
| ] | ||
|
|
||
| self.execute_test( | ||
| get_crash_event_with_frames(frames), | ||
| False, # Should NOT be reported - SDK frame below doesn't count | ||
| mock_sdk_crash_reporter, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I could have been more specific in #105625. We should rather overreport than underreport SDK crashes. I think we should still report that crash as an SDK crash. I think the logic should be it's OK to ignore the SDK crash if the specific function is the only SDK frame in the whole stacktrace.
|
@philipphofmann I've fixed according to your feedback, could you review it again? |
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including the feedback. We're not far away from an approval, @calm329 💯
| If there are other SDK frames above these frames, the crash is still reported as an SDK crash. | ||
| For example, SentrySwizzleWrapper is used for method swizzling and shouldn't be reported as an SDK crash | ||
| when it's the only SDK frame, since the actual crash originates from system libraries.""" | ||
| These frames are typically SDK instrumentation frames that intercept calls, such as swizzling or monkey patching, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's better thanks
| def _matches_ignore_when_only_sdk_frame(self, frame: Mapping[str, Any]) -> bool: | ||
| """ | ||
| Returns true if the frame matches the sdk_crash_ignore_when_only_sdk_frame_matchers pattern. | ||
| """ | ||
| function = frame.get("function") | ||
| if not function: | ||
| return False | ||
|
|
||
| module = frame.get("module") | ||
| for matcher in self.config.sdk_crash_ignore_when_only_sdk_frame_matchers: | ||
| function_matches = glob_match(function, matcher.function_pattern, ignorecase=True) | ||
| module_matches = glob_match(module, matcher.module_pattern, ignorecase=True) | ||
|
|
||
| if function_matches and module_matches: | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
| def _matches_sdk_crash_ignore(self, frame: Mapping[str, Any]) -> bool: | ||
| function = frame.get("function") | ||
| if not function: | ||
| return False | ||
|
|
||
| module = frame.get("module") | ||
| for matcher in self.config.sdk_crash_ignore_matchers: | ||
| function_matches = glob_match(function, matcher.function_pattern, ignorecase=True) | ||
| module_matches = glob_match(module, matcher.module_pattern, ignorecase=True) | ||
|
|
||
| if function_matches and module_matches: | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: These two methods are almost the same. It would be great to get rid of the code duplication, please.
| ) | ||
|
|
||
|
|
||
| @patch("sentry.utils.sdk_crashes.sdk_crash_detection.sdk_crash_detection.sdk_crash_reporter") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tests.
| has_non_conditional_sdk_frame = False | ||
| for frame in iter_frames: | ||
| if ( | ||
| self.is_sdk_frame(frame) | ||
| and not self._matches_ignore_when_only_sdk_frame(frame) | ||
| and not self._matches_sdk_crash_ignore(frame) | ||
| ): | ||
| has_non_conditional_sdk_frame = True | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: I find this a bit confusing now. We have a method called _matches_sdk_crash_ignore, but we don't use it below. We still have code that calls for matcher in self.config.sdk_crash_ignore_matchers below. This code for matcher in self.config.sdk_crash_ignore_matchers below should also use the _matches_sdk_crash_ignore method please.
| # are SDK frames or from system libraries. | ||
| iter_frames = [f for f in reversed(frames) if f is not None] | ||
|
|
||
| has_non_conditional_sdk_frame = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: I think this code could more clearly communicate what it does. We can iterate over all frames 3 times, so it's still O(n), where n is the number of frames.
First, I would iterate over all frames to check if we should ignore the crash altogether, because a frame matches the _matches_sdk_crash_ignore. If it does, we ignore the SDK crash.
Second, I would iterate over all frames to check if a grame matches the _matches_ignore_when_only_sdk_frame. This loop keeps track if it's the only SDK frame that matches this condidition. If it does then we exit and say, no SDK crash.
The third loop performs the remaining checks to determine whether it's an SDK crash. Then we have the logic seperated into three different for loops and they don't overlap with each other in one for loop, which makes it now harder to understand.
Fixes #105625
Summary
The SDK crash detection was incorrectly reporting crashes as SDK crashes when
SentrySwizzleWrapperwas the only SDK frame in the stacktrace. These are false positives becauseSentrySwizzleWrapperis an instrumentation frame that intercepts UI events but doesn't cause crashes itself.This PR adds a new conditional SDK frame detection mechanism that only ignores
SentrySwizzleWrapperwhen there are no other SDK frames above it (closer to the crash origin).Changes
sdk_crash_ignore_when_only_sdk_frame_matchersfield toSDKCrashDetectionConfigis_sdk_crash()inSDKCrashDetectorto handle conditional SDK framesSentrySwizzleWrapperpattern to Cocoa SDK configurationCocoaSDKSwizzleWrapperTestMixinwith 3 test cases:How it works
When iterating through frames (youngest to oldest):
True(SDK crash)False(not SDK crash, since any SDK frames above would have triggered step 1)FalseLegal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=148254234