Fix potential NPE in UIViewOperationQueue.

[manicakes]

We were noticing the following crash in our application, that was occurring fairly often, but still hard to reproduce:

12-12 10:37:35.342: E/AndroidRuntime(9064): FATAL EXCEPTION: main
12-12 10:37:35.342: E/AndroidRuntime(9064): Process: com.bloomberg.android.plus, PID: 9064
12-12 10:37:35.342: E/AndroidRuntime(9064): java.lang.NullPointerException: Attempt to invoke interface method 'void com.facebook.react.uimanager.UIViewOperationQueue$UIOperation.execute()' on a null object reference
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at com.facebook.react.uimanager.UIViewOperationQueue$2.run(UIViewOperationQueue.java:782)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at com.facebook.react.uimanager.UIViewOperationQueue.flushPendingBatches(UIViewOperationQueue.java:829)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at com.facebook.react.uimanager.UIViewOperationQueue.access$1500(UIViewOperationQueue.java:44)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at com.facebook.react.uimanager.UIViewOperationQueue$DispatchUIFrameCallback.doFrameGuarded(UIViewOperationQueue.java:868)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at com.facebook.react.uimanager.GuardedChoreographerFrameCallback.doFrame(GuardedChoreographerFrameCallback.java:32)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at com.facebook.react.uimanager.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:131)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:872)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at android.view.Choreographer.doCallbacks(Choreographer.java:686)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at android.view.Choreographer.doFrame(Choreographer.java:618)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:860)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at android.os.Handler.handleCallback(Handler.java:751)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at android.os.Handler.dispatchMessage(Handler.java:95)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at android.os.Looper.loop(Looper.java:154)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at android.app.ActivityThread.main(ActivityThread.java:6088)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at java.lang.reflect.Method.invoke(Native Method)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
12-12 10:37:35.342: E/AndroidRuntime(9064): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

Looks like we may have null objects in the UIViewOperationQueue. I went ahead and put in null checks in the relevant places.

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @astreet and @gre to be potential reviewers.

[gre]

looks good to me

[astreet]

This would imply that somewhere we're adding null to the queue. A better fix would be to never allow null to be added to the queue -- there's no reason I can think of that we'd ever want that.

[manicakes]

@astreet makes sense, though a bit time constrained at the moment, probably won't be able to find the root cause anytime soon. Shall I close this PR and open an issue?

[astreet]

There's nothing that I can see that could add null to the operations queue in open source -- do you have any non-open source usages of UIViewOperationQueue#enqueueUIOperation? All other add calls I can see add a new object that can't be null. I'm worried we might be hiding a bug somewhere (in our code or yours) or could hide a bug in the future.

[manicakes]

@astreet I think this is worth investigating. Our code is unfortunately not open source, but as soon as I can reproduce this consistently, I will either fix the bug or put together a sample app that causes the error.

[manicakes]

So weird - I'm not seeing any calls to enqueueUIOperation other than the method definition. Grepping our entire source tree, including imported modules:

$ find . -type f -name "*.java" -exec grep -il enqueueUIOperation {} \;
./node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIViewOperationQueue.java

[astreet]

Alright, thanks for following up. Not saying it can't somehow be a bug in OSS, but we haven't had this crop up for any of our apps at FB AFAICT

[manicakes]

NP - will try to spend some time on this today and hopefully will find something. Cheers.

[manicakes]

@astreet unfortunately didn't get to the bottom of this. However, I updated this PR to include a null check for the enqueueUIOperation method; thinking that should be there anyway. Whether it fixes the NPE is another matter. :) (PR title updated to reflect this)

[manicakes]

@astreet any thoughts on this PR? This is currently our most common crash on Android, so your help is much appreciated.

[astreet]

Instead of a null-check, I'm fine adding SoftAssertions.assertNotNull to enqueueUIOperation: this should catch the problem earlier and give you a better stack trace.

[astreet]

Remove this change since it's not relevant.

[manicakes]

@astreet updated, thanks!

[astreet]

@facebook-github-bot shipit

[astreet]

Thanks!

[dmueller39]

@astreet After reexamining this problem this issue isn't due to adding null to mOperations, its due to mutating mOperations from the main UI thread. The SoftAssertion here probably isn't doing anything. I'd like to add SOMETHING to help people in the future, but I'm not certain what.

[dmueller39]

Actually the issues isn't the main thread, its any thread other than the native_modules thread. asserting on UiThreadUtil.isOnUiThread() would catch 95% of cases, but not 100% of cases (if say, someone spins up their own thread) But given that this problem seems to be pretty uncommon, maybe its enough.

[evelant]

This is happening to me @dmueller39 and It's driving me mad. I opened #27819. I see no way that mOperations can get a null reference put into it. I searched my whole node_modules looking for things that call enqueueUIOperation and there are none. This crash is happening for lots of my users =(

[evelant]

@manicakes I had to fork and re-add your change of if(op !== null) since things were still crashing frequently without it. As @dmueller39 said the SoftAssertion doesn't appear to do anything. I still haven't been able to figure out the root cause of the issue, but my app seems to run fine with the extra null check.

[MAKARD]

We're getting the same issue with our app. We can definitely say that it crashes when users landed to webview. It could be one minute after land, or it could be 2 seconds after land (according to dynatrace session recording).