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: race condition when checking current activity #30

Merged
merged 1 commit into from
Oct 31, 2022
Merged

fix: race condition when checking current activity #30

merged 1 commit into from
Oct 31, 2022

Conversation

zolbooo
Copy link
Contributor

@zolbooo zolbooo commented Oct 31, 2022

We were experiencing some crashes caused by the NullPointerException raised in the library in production.

java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.Window android.app.Activity.getWindow()' on a null object reference
    at com.reactnativesystemnavigationbar.SystemNavigationBarModule.setNavigationColor(SystemNavigationBarModule.java:1)
    at java.lang.reflect.Method.invoke(Method.java)
    at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:18)
    at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:13)
    at com.facebook.react.bridge.queue.NativeRunnable.run(NativeRunnable.java)
    at android.os.Handler.handleCallback(Handler.java:938)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:1)
    at android.os.Looper.loopOnce(Looper.java:210)
    at android.os.Looper.loop(Looper.java:299)
    at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(MessageQueueThreadImpl.java:8)
    at java.lang.Thread.run(Thread.java:920)

It appears that Activity was stopped before setNavigationColor call.

- ui.lifecycle {
  screen: MainActivity, 
  state: stopped
}
- ui.lifecycle {
  screen: MainActivity, 
  state: saveInstanceState
}
- NullPointerException: Attempt to invoke virtual method 'android.view.Window android.app.Activity.getWindow()' on a null object reference

There is an issue in the code below:

if (getCurrentActivity() == null) {
         promise.reject("Error: ", "false");
         return;
}
final Window view = getCurrentActivity().getWindow();

Because getCurrentActivity() result is not saved, current activity may be replaced by null after if condition worked, which is exactly what happens before the crash. For example,
Happy path: App working -> setNavigationColor() -> app stops -> if getCurrentActivity() == null is true -> return.
Edge case: App working -> setNavigationColor() -> if getCurrentActivity() == null is FALSE -> app stops -> getCurrentActivity().getWindow() -> NPE.

In the edge case NPE happens because app is already stopped, so that getCurrentActivity() returns null and then getWindow() is called on the null instance.

This PR saves getCurrentActivity() result into the variable, so that Activity.getWindow() never raises a NPE.

We should save currentActivity to a variable,
as getCurrentActivity() may return null on the subsequent calls.
@kadiraydinli kadiraydinli merged commit 795827d into kadiraydinli:master Oct 31, 2022
@kadiraydinli
Copy link
Owner

Thanks for PR.

@zolbooo zolbooo deleted the fix/get-activity-race-condition branch October 31, 2022 18:17
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.

None yet

2 participants