-
Notifications
You must be signed in to change notification settings - Fork 207
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
Fix NPE causing crash when reporting a minimal error #534
Conversation
…ttempting to extract info from When a corrupted report is stored from a previous version of bugsnag-android on disk, information required to construct a minimal error report is not present in the filename. This means the Error will be returned as null - causing a NPE when attempting to notify. This adds a null check to fix this scenario.
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.
Looks good, works as specified. I tested without your changeset and reproduced the problem. Testing with your changeset I could no longer reproduce the npe.
A couple of things to note:
-
when the app boots with a bad file, it outputs "sending 1 saved error" twice:
07-23 14:50:58.197 27016 27033 I Bugsnag : Sending 1 saved error(s) to Bugsnag 07-23 14:50:58.207 27016 27033 I Bugsnag : Sending 1 saved error(s) to Bugsnag 07-23 14:50:58.263 27016 27016 I Bugsnag : Initialised NDK Plugin 07-23 14:50:58.270 27016 27016 I Bugsnag : Initialised ANR Plugin 07-23 14:50:58.778 27016 27033 I Bugsnag : Completed session tracking request
-
the offending file isn't removed from the device's filesystem
I think this may be an issue with adb not synchronising the file directory, which we observed last time when testing the original minimal error implementation. The file is deleted here: https://github.com/bugsnag/bugsnag-android/pull/534/files#diff-5fcb9931669c81ccf180176a177f999eR180 I will investigate why that message shows up twice (and also make a small fix to the mazerunner tests) |
I'm not sure about that… if repeatedly open/close the app (without making it crash again) it says this every time:
This suggests to me that the file isn't removed? (in addition to seeing it in the device file explorer) |
d9230df
to
055cffa
Compare
I've done the following testing:
In all cases the file was deleted after the app was launched, and I did not observe |
This was down to some weirdness with my emulator state. I tested on a different one (with a different api level) and it worked. @fractalwrench tested on an emulator with the same api level as my weird on (26) and it also worked. |
055cffa
to
10b260b
Compare
10b260b
to
b4a30d2
Compare
Goal
v4.16.1 added reporting of minimal error information, by encoding key information such as handled state in the filename. If the JSON payload was corrupted in any way, this means an incomplete report could be delivered with at least some information.
If an older corrupted report was cached on disk this would return a
null
error as the information was not encoded in the filename. This changeset adds a null check to prevent an NPE in thenotify()
call.Tests
Added a mazerunner scenario that creates an empty file with an old filename that does not contain encoded information. The notifier should not generate a report from this file and should also not crash.