Fix NPE for outcomeEventsController during initialization
#1539
Conversation
- Use lazy initialization and singleton pattern to access and create outcomeEventsController - When we call outcomeEventsController's methods, check if it is null, and if so, initialize it - Follow https://www.infoworld.com/article/2077568/java-tip-67--lazy-instantiation.html
nan-li
changed the title
outcomeEventsController during initializationoutcomeEventsController during initialization
There was a problem hiding this comment.
Logic changes look good, have a concern about the shadow changes though and it effecting other tests.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jeasmine and @nan-li)
OneSignalSDK/unittest/src/test/java/com/onesignal/ShadowOneSignal.java, line 30 at r1 (raw file):
*/ @Implementation public static void setupContextListeners(boolean wasAppContextNull) {
There are a number of tests using this shadow, this method changes the behavior quite a bit. Can we confirm if this change is needed at all?
If it is could we do one of the following?:
- By default call the original logic, we could add an if statement to check a flag and only set it in the test that needs this.
- Create a different class that shadows the OneSignal class and only use it in the test you need.
- Add unit test: initWithContext_setupContextListenersNotCompleted_doesNotProduceNPE to reproduce the NPE of `outcomeEventsController` in `init()`. - Shadow the `OneSignal.setupContextListeners()` method to simulate it not completing (thus not initializing the `outcomeEventsController`). - Add new shadow of OneSignal besides ShadowOneSignal named `ShadowOneSignalWithMockSetupContextListeners` so other unit tests using `ShadowOneSignal` don't have behavior changed - However, the `languageContext` initialization is needed or else it leads to a subsequent NPE for this property in `registerUserTask()`. - Change access of `languageContext` in OneSignal.java from private to package-private so it can be accessed in unit tests.
nan-li
force-pushed
the
fix/outcomeEventsController_init_npe
branch
from
2cfe353
to
fd9c957
Compare
Good catch, I've created a new shadow class |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @emawby and @Jeasmine)
There was a problem hiding this comment.
Reviewable status:
|
Tests failed but they are most likely flaky unrelated ones: Test 2: Rerunning the tests to confirm. |
|
Running the unit tests locally twice, they all pass. |
Description
One Line Summary
Lazily initialize and access the
OneSignal.outcomeEventsController, adding a null check when invoking its methods that wasn't checked before, as it may not have been initialized yet in the critical initialization step ofsetupContextListeners().Details
Motivation
This is a speculative fix for a non-reproducible
NullPointerExceptionofoutcomeEventsController.sendSavedOutcomes()during initialization, as reported by users in production here and here.Context
During initialization,
initWithContext()andsetAppId()run. The first method also initializes theoutcomeEventsControllerin the methodsetupContextListeners()see code.After the appContext and appId are set by the above methods, the
init()method runs, which callsOSOutcomeEventsController.sendSavedOutcomes()towards the method's end.It may be that in some cases, the
init()method is called beforeinitWithContext()is fully complete, resulting in a nulloutcomeEventsControllerinstance thatinit()relies on.One GitHub reporter speculated it is due to adding
setLanguagefunctionality in4.4.0, and there are now more calls toinitWithContextadded in4.4.1for more FCM entry points.It is possible that multiple threads are initializing and one hangs after setting the
appContextbut hasn't finishedsetupContextListeners.This is ultimately speculative and it may be some other reason that
outcomeEventsControllerisnull.Implementation Details
When we want to access
OneSignal.outcomeEventsController, use lazy initialization and singleton pattern to access and create it viagetOutcomeEventsController(). Use synchronization when creating the object.There are some methods where
outcomeEventsControlleris called such as insendUniqueOutcome()where there is already anullcheck and the method returns ifoutcomeEventsController == null. In these methods I didn’t make any changes as it already had its own null check logic.Scope
This affects OneSignal initialization.
Potential Problems
If the root issue is in fact that
setupContextListeners()isn't completed beforeinit()runs, it may be possible thatlanguageContextisn't initialized either. In this case, while we move past the NPE foroutcomeEventsController, we may encounter a NPE forlanguageContextnext, such as inregisterUserTask()or when it is passed as an argument togetInAppMessageController().However, that may be far enough down the line that it won't pose a problem.
Testing
Unit testing
Added unit test to simulate
setupContextListeners()not completing:Add unit test
initWithContext_setupContextListenersNotCompleted_doesNotProduceNPEto reproduce the NPE ofoutcomeEventsControllerininit().Shadow the
OneSignal.setupContextListeners()method to simulate it not completing (thus not initializing theoutcomeEventsController).However, the
languageContextinitialization is needed or else it leads to a subsequent NPE for this property inregisterUserTask().Change access of
languageContextin OneSignal.java from private to package-private so it can be accessed in unit tests.All other unit tests pass
Manual testing
App built with Android Studio 2020.3.1 with the OneSignal example app on a Pixel 5 with API 30.
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is