Skip to content

Fix NPE: use a getter for the current UserState#1590

Merged
nan-li merged 2 commits intomainfrom
fix/npe_set_email
May 17, 2022
Merged

Fix NPE: use a getter for the current UserState#1590
nan-li merged 2 commits intomainfrom
fix/npe_set_email

Conversation

@nan-li
Copy link
Copy Markdown
Contributor

@nan-li nan-li commented May 16, 2022

Description

One Line Summary

Use a getter for the currentUserState to avoid a null pointer exception.

Details

Motivation

A customer reported 3 separate crashes for the app's users when (1) a device is logging out of email, or, (2) has logged out of email and is logging into a different email. The currentUserState is apparently null. To address these crash reports, replace currentUserState with getCurrentUserState() to avoid these NPE's.

I also replaced more instances of currentUserState with getCurrentUserState() than the 3 cases below reported by the user. These are other places in UserStateSynchronizer that access currentUserState and it could possibly be null.

Crash 1: Not reproducible

The obfuscated method seems to be this line.

Fatal Exception: java.lang.NullPointerException: 
Attempt to invoke virtual method 'i.g.u i.g.k4.e()' on a null object reference // probably currentUserState.getDependValues();
      at com.onesignal.UserStateSynchronizer.doEmailLogout(UserStateSynchronizer.java:261)
      at com.onesignal.UserStateSynchronizer.internalSyncUserState(UserStateSynchronizer.java:227)
      at com.onesignal.UserStateSynchronizer.syncUserState(UserStateSynchronizer.java:219)
      at com.onesignal.UserStateSynchronizer$NetworkHandlerThread$1.run(UserStateSynchronizer.java:110)
      at android.os.Handler.handleCallback(Handler.java:938)
      at android.os.Handler.dispatchMessage(Handler.java:99)
      at android.os.Looper.loopOnce(Looper.java:226)
      at android.os.Looper.loop(Looper.java:313)
      at android.os.HandlerThread.run(HandlerThread.java:67)

Crash 2: Not reproducible

Attempt to invoke virtual method 'org.json.JSONObject com.onesignal.UserState.generateJsonDiff(com.onesignal.UserState, boolean)' on a null object reference

Crash 3: Reproducible by the reporter, not reproducible on my end

java.lang.NullPointerException: Attempt to invoke virtual method
'void com.onesignal.UserState.setSyncValues(org.json.JSONObject)' on a null object reference
        at com.onesignal.UserStateSynchronizer.resetCurrentState(UserStateSynchronizer.java:573)
        at com.onesignal.UserStateSecondaryChannelSynchronizer.setChannelId(UserStateSecondaryChannelSynchronizer.java:156)
        at com.onesignal.OneSignalStateSynchronizer.setEmail(OneSignalStateSynchronizer.java:195)
        at com.onesignal.OneSignal.setEmail(OneSignal.java:1699)
        at com.onesignal.OneSignal.setEmail(OneSignal.java:1640)

Scope

In the places we were accessing currentUserState, it should not be null anyway. Using a getter just ensures this, and creates it if it is null.

Testing

Unit testing

No new unit testing done.

Manual testing

Because I couldn't reproduce any of these exceptions, it was not testable.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes
  • Device State

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

* The currentUserState could be null from a user's report

* Replace currentUserState with getCurrentUserState() to avoid an NPE

* Replaced more instances of currentUserState with getCurrentUserState() than just the cases reported by the user. Which are all the places in UserStateSynchronizer that access currentUserState and it could possibly be null.
@nan-li nan-li marked this pull request as ready for review May 16, 2022 23:43
@nan-li nan-li requested a review from a team May 16, 2022 23:44
Copy link
Copy Markdown
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.

Thinking we should change all currentUserState usages to getCurrentUserState() for consistency.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nan-li)

* In the previous commit, we already replaced all access to `currentUserState` where it could possibly be `null`, with the getter `getCurrentUserState()`.

* In this commit, replace all other access to `currentUserState` with the getter `getCurrentUserState()` for consistency purposes.
Copy link
Copy Markdown
Contributor Author

@nan-li nan-li left a comment

Choose a reason for hiding this comment

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

Ok! Added another commit for this so that the first commit is to address the NPE specifically, and the second commit, for consistency.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @jkasten2)

Copy link
Copy Markdown
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.

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nan-li)

@nan-li nan-li merged commit 538e993 into main May 17, 2022
@nan-li nan-li deleted the fix/npe_set_email branch May 17, 2022 22:40
@jkasten2 jkasten2 mentioned this pull request May 18, 2022
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.

2 participants