-
Notifications
You must be signed in to change notification settings - Fork 376
fix(crash): NPE in PermissionsActivity #2349
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
Conversation
* This should never happen as the SDK does send extras for this Activity, and no other entity should be creating PermissionsActivity. * However, we have seen crash reports from our users that primarily seems to affect Nexus 5 and Pixel 3, in low numbers. * One theory is that automatic testing done by the google play store is explicitly driving the PermissionsActivity.
|
||
if (intent.extras == null) { | ||
// This should never happen, but extras is null in rare crash reports | ||
return |
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.
Returning early keeps the Activity alive (it will still hit onStart/onResume), leaves it on the back stack, and can cause a blank/flickery frame. Even though this was most likely triggered by tests passing null extras, we should harden production code.
I would recommend reusing the existing exit path at line 49 so behavior is deterministic.
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.
This is because we already called super.onCreate()
so we need to call finish()
?
Should the initWithContext check do that as well? I think it is more common for initWithContext to return false, than for the extras to be null?
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.
In practice, I suspect the initWithContext check never fail: showing the permission prompt requires OneSignal.getNotification()
or OneSignal.getLocation()
, and if initWithContext wasn’t invoked earlier, the “must call initWithContext before use”
exception would already be thrown.
For the case of the automated tests, initializing OneSignal by calling initWithContext in onCreate()
here seems to work, so was continued to handleBundleParams()
.
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.
Let's call finish on all early returns, updated the PR.
* Prevent flicker or blocking, finish activity before returning early
Description
One Line Summary
Fix null pointer exception when PermissionsActivity is being created.
Details
Motivation
Scope
extras
isnull
, then we return early, which is safe to do as it doesn't make sense to show the Activity anyway.Testing
Unit testing
None
Manual testing
None, unable to repro
From Josh:
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is