fix: Add userInfo context for unhandled NSExceptions#8332
Conversation
📲 Install BuildsiOS
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| e983818 | 1231.33 ms | 1265.04 ms | 33.72 ms |
| dea87d0 | 1228.73 ms | 1270.64 ms | 41.91 ms |
| 6c524d8 | 1217.83 ms | 1254.02 ms | 36.20 ms |
| 047965c | 1219.57 ms | 1252.02 ms | 32.45 ms |
| bbc91e5 | 1215.94 ms | 1243.26 ms | 27.32 ms |
| 36c25b3 | 1229.10 ms | 1255.83 ms | 26.73 ms |
| adef457 | 1229.45 ms | 1262.67 ms | 33.22 ms |
| 9319185 | 1223.24 ms | 1249.98 ms | 26.73 ms |
| 119dc37 | 1217.64 ms | 1250.44 ms | 32.80 ms |
| 49cc12d | 1227.33 ms | 1263.21 ms | 35.89 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| e983818 | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| dea87d0 | 24.14 KiB | 1.23 MiB | 1.20 MiB |
| 6c524d8 | 24.14 KiB | 1.15 MiB | 1.12 MiB |
| 047965c | 24.14 KiB | 1.22 MiB | 1.20 MiB |
| bbc91e5 | 24.14 KiB | 1.17 MiB | 1.14 MiB |
| 36c25b3 | 24.14 KiB | 1.18 MiB | 1.16 MiB |
| adef457 | 24.14 KiB | 1.15 MiB | 1.13 MiB |
| 9319185 | 24.14 KiB | 1.16 MiB | 1.13 MiB |
| 119dc37 | 24.14 KiB | 1.16 MiB | 1.13 MiB |
| 49cc12d | 24.14 KiB | 1.16 MiB | 1.13 MiB |
Previous results on branch: supervacuus/fix/userInfo_context_unhandled_NSException
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 78b510c | 1215.23 ms | 1244.23 ms | 29.00 ms |
| c199537 | 1213.65 ms | 1258.60 ms | 44.94 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 78b510c | 24.14 KiB | 1.23 MiB | 1.20 MiB |
| c199537 | 24.14 KiB | 1.22 MiB | 1.20 MiB |
philprime
left a comment
There was a problem hiding this comment.
LGTM but we need to add more testing (summarized with assistance of LLM agents) to cover most of the tests only cover happy paths of the (OpenStep string → parsed dictionary) parsing.
That said, contextUserInfoFromNSExceptionUserInfo: has several branches with no explicit coverage:
userInfois"(null)"or empty string""userInfois a non-parseable string → fallback to@{ @"NSException.userInfo" : str }userInfois some other type (returnsnil)
The NSExceptionWithoutReason.json fixture actually contains a userInfo OpenStep string ("{\n detail = \"Additional information\";\n}"), so testNSExceptionWithoutReason exercises the parsing path too, but doesn't assert anything about event.context[@"user info"].
The other branches (nil, empty, wrong type) are defensive guards that are unlikely to regress, so lower priority, but mentioned for completeness.
Added more tests to cover the raised behavior and a few additional ones to encode behavior wrt overrides, merging and other edge cases. |
| id propertyList = [NSPropertyListSerialization propertyListWithData:data | ||
| options:NSPropertyListImmutable | ||
| format:nil | ||
| error:nil]; |
There was a problem hiding this comment.
h: We are silently ignoring the error here. We should properly propagate it to the caller and at least add a SentrySDKLog.warning/error statement
📜 Description
This PR maps
crash.error.nsexception.userInfo(which both SentryCrash and KSCrash write) onto the Sentry event context.💡 Motivation and Context
Fixes #5484.
Unhandled
NSExceptioncrash reports already containNSException.userInfo, butSentryCrashReportConverteronly converted the exception name/reason into the event.This meant that fatal/unhandled NSExceptions lost
userInfo, while handled NSExceptions captured throughSentryClientpreserved it in the event context"user info".💚 How did you test it?
Added a unit test using a mock report and also adapted the existing NSException e2e crash test case (which was the trigger for implementing the feature in the first place).
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.