fix: Serialization with invalid objects #2858
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7fb7afb | 1202.18 ms | 1219.42 ms | 17.24 ms |
| 9e96fd6 | 1207.20 ms | 1229.40 ms | 22.20 ms |
| 984eb2d | 1220.62 ms | 1235.24 ms | 14.62 ms |
| d60f70a | 1219.63 ms | 1228.54 ms | 8.91 ms |
| e8b2fb4 | 1230.70 ms | 1242.84 ms | 12.14 ms |
| 28333b6 | 1186.29 ms | 1225.18 ms | 38.89 ms |
| 25bcc50 | 1237.69 ms | 1258.40 ms | 20.71 ms |
| 3389927 | 1230.12 ms | 1238.04 ms | 7.92 ms |
| ddc9b9a | 1210.00 ms | 1234.18 ms | 24.18 ms |
| 67460f4 | 1244.56 ms | 1255.96 ms | 11.40 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7fb7afb | 20.76 KiB | 419.70 KiB | 398.94 KiB |
| 9e96fd6 | 20.76 KiB | 425.80 KiB | 405.04 KiB |
| 984eb2d | 20.76 KiB | 425.77 KiB | 405.01 KiB |
| d60f70a | 20.76 KiB | 430.97 KiB | 410.21 KiB |
| e8b2fb4 | 20.76 KiB | 427.23 KiB | 406.46 KiB |
| 28333b6 | 20.76 KiB | 424.69 KiB | 403.93 KiB |
| 25bcc50 | 20.76 KiB | 427.23 KiB | 406.46 KiB |
| 3389927 | 20.76 KiB | 427.23 KiB | 406.46 KiB |
| ddc9b9a | 20.76 KiB | 420.40 KiB | 399.64 KiB |
| 67460f4 | 20.76 KiB | 426.15 KiB | 405.39 KiB |
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
Thanks, @brustolin. LGTM
Co-authored-by: Philipp Hofmann <[email protected]>
… into fix/serialization
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2858 +/- ##
==========================================
+ Coverage 81.35% 81.47% +0.11%
==========================================
Files 260 260
Lines 24367 24367
Branches 10793 10807 +14
==========================================
+ Hits 19823 19852 +29
+ Misses 4040 4013 -27
+ Partials 504 502 -2
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| // We'll do this whether we're handling an exception or library error | ||
| #define SENTRY_HANDLE_ERROR(__sentry_error) \ | ||
| SENTRY_LOG_ERROR(@"Invalid JSON: %@", __sentry_error); \ | ||
| *error = __sentry_error; \ | ||
| return nil; | ||
|
|
||
| NSData *data = nil; | ||
|
|
||
| #if defined(DEBUG) || defined(TEST) || defined(TESTCI) | ||
| @try { | ||
| #else | ||
| if ([NSJSONSerialization isValidJSONObject:dictionary]) { | ||
| #endif | ||
| data = [NSJSONSerialization dataWithJSONObject:dictionary options:0 error:error]; | ||
| #if defined(DEBUG) || defined(TEST) || defined(TESTCI) | ||
| } @catch (NSException *exception) { | ||
| if (error) { | ||
| SENTRY_HANDLE_ERROR(NSErrorFromSentryErrorWithException( | ||
| kSentryErrorJsonConversionError, @"Event cannot be converted to JSON", exception)); | ||
| } |
There was a problem hiding this comment.
I understand this code was weird, but it did help me to diagnose some json serialization issues in the past. isValidJSONObject only returns YES or NO but the exception tells you what is invalid. Maybe we should consider just always doing the @try/@catch even in production and including that info in any errors we send to ourselves.
There was a problem hiding this comment.
We had to rewrite this part of the code to be able to understand it and solve a bug that affects everyone, and ultimately, the CI code was not the same as the release code, that was why we did not detect it early.
AFAIK is a good practice to avoid @try/catch for objc when possible.
But I agree that having the actual error description would help a lot, something we dont get with isValidJSONObject.
@philipphofmann any thoughts?
There was a problem hiding this comment.
Agreed it's good practice to avoid @try/@catch, but it was only compiled that way for non-prod. It was only used in debug/test to help us diagnose things during development.
I'd be surprised if we ultimately were seeing different outcomes between catching an exception or looking at the inout error, vs calling isValidJSONObject, as the documentation states either method is appropriate to validate the input.
There was a problem hiding this comment.
The error was fixed, now we have a better understanding of what was happening, I don't see a problem to use the compiler declarative again, as long as we don't make a "spaghetti" code, is hard to understand.
Is ok to duplicate code to have only one #if/#else, way better to read the code.
There was a problem hiding this comment.
Using compiler directives led to having untested code, which led to crashes in production. If we want extra context when debugging I would prefer using something like options.debug and test all the code properly. Then our users would also see the extra context when they are debugging.
📜 Description
Serialization may fail and crash if there is an invalid objects in the data.
Related PR #2355.
💡 Motivation and Context
May be related to #2850
💚 How did you test it?
Unit tests
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.