Skip to content
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 possible NullPointerException under notValidOrDuplicated #1372

Merged
merged 2 commits into from
Jul 6, 2021

Conversation

Jeasmine
Copy link
Contributor

@Jeasmine Jeasmine commented Jun 30, 2021

NullPointerException was happening when calling notValidOrDuplicated under an OSNotificationDataController not inited; this can occur if for some reason OSNotificationDataController got cleaned from memory, or the notification processing doesn't call initWithContext.

Task:

  • Init notificationDataController if controller is null
  • Add description to notValidOrDuplicated method

Fixes #1357


This change is Reviewable

@Jeasmine Jeasmine requested review from jkasten2 and emawby June 30, 2021 20:57
* notValidOrDuplicated is being called from notification processing, initWithContext might no be called
* Init notificationDataController if controller is null
* Add description to notValidOrDuplicated method
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be calling OneSignal.initWithContext(context) internally already which initializes notificationDataController already. We may have just missed one spot. Possibly just a Service as the app's process could have been started from directly in some rare cases.

Adding OneSignal.initWithContext(context) will end up call setupContextListeners so just adding this where needed should fix the issue. Doing this means we won't have future issues where something else could be missed.

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @emawby)

* From every FCM entry point we should have initWithContext call
Copy link
Contributor Author

@Jeasmine Jeasmine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wanted to avoid searching for every place since we should have one place to handle this, the initWithContext from notification handling. I have seen that most of the notification processing passes through processBundleFromReceiver, so we might want to let that function handle the init.

I will still leave the null controller check since it will avoid future problems also, and if we have the context available, we should use it.

Reviewable status: 2 of 6 files reviewed, all discussions resolved (waiting on @emawby and @jkasten2)

@Jeasmine Jeasmine requested a review from jkasten2 July 1, 2021 20:06
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @emawby)

@Jeasmine Jeasmine merged commit 526f8f0 into main Jul 6, 2021
@Jeasmine Jeasmine deleted the fix/npe-validorduplicate branch July 6, 2021 18:12
@Jeasmine Jeasmine mentioned this pull request Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OneSignal.java line 3063 com.onesignal.OneSignal.notValidOrDuplicated
2 participants