From 980900c6343b93259e46edd44b6f267aa534cde5 Mon Sep 17 00:00:00 2001
From: Joshua Gross <joshuagross@fb.com>
Date: Fri, 17 Jul 2020 01:04:00 -0700
Subject: [PATCH] Workaround for NativeAnimatedModule queue race condition

Summary:
Apparently it's possible for `!isEmpty()` to be true and `peek` to be non-null, but for `poll()` to be false. It doesn't really make sense to me, and I don't have a repro, but that's what logs are showing.

I suspect a teardown race condition or /maybe/ a Fabric/non-Fabric handoff race condition. Neither of those make a ton of sense, though.

The mitigation is fairly straightforward: we are just much more cautious with how we treat the queue and assume everything could be null.

This impacts Fabric and non-Fabric equally.

Changelog: [Internal]

Reviewed By: motiz88

Differential Revision: D22593924

fbshipit-source-id: 7748121951a64941fa6da2bd25ebf070be6dc89c
---
 .../react/animated/NativeAnimatedModule.java  | 36 ++++++++++++++++---
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java
index 46e2b78f311..249bf377589 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java
@@ -104,10 +104,12 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec
   private final ReactChoreographer mReactChoreographer;
 
   @NonNull
-  private ConcurrentLinkedQueue<UIThreadOperation> mOperations = new ConcurrentLinkedQueue<>();
+  private final ConcurrentLinkedQueue<UIThreadOperation> mOperations =
+      new ConcurrentLinkedQueue<>();
 
   @NonNull
-  private ConcurrentLinkedQueue<UIThreadOperation> mPreOperations = new ConcurrentLinkedQueue<>();
+  private final ConcurrentLinkedQueue<UIThreadOperation> mPreOperations =
+      new ConcurrentLinkedQueue<>();
 
   private @Nullable NativeAnimatedNodesManager mNodesManager;
 
@@ -226,8 +228,34 @@ public class NativeAnimatedModule extends NativeAnimatedModuleSpec
 
   private void executeAllOperations(Queue<UIThreadOperation> operationQueue, long maxFrameNumber) {
     NativeAnimatedNodesManager nodesManager = getNodesManager();
-    while (!operationQueue.isEmpty() && operationQueue.peek().getFrameNumber() <= maxFrameNumber) {
-      operationQueue.poll().execute(nodesManager);
+    while (true) {
+      // There is a race condition where `peek` may return a non-null value and isEmpty() is false,
+      // but `poll` returns a null value - it's not clear why since we only peek and poll on the UI
+      // thread, but it might be something that happens during teardown or a crash. Regardless, the
+      // root cause is not currently known so we're extra cautious here.
+      // It happens equally in Fabric and non-Fabric.
+      UIThreadOperation peekedOperation = operationQueue.peek();
+
+      // This is the same as operationQueue.isEmpty()
+      if (peekedOperation == null) {
+        return;
+      }
+      // The rest of the operations are for the next frame.
+      if (peekedOperation.getFrameNumber() > maxFrameNumber) {
+        return;
+      }
+
+      // Since we apparently can't guarantee that there is still an operation on the queue,
+      // much less the same operation, we do a poll and another null check. If this isn't
+      // the same operation as the peeked operation, we can't do anything about it - we still
+      // need to execute it, we have no mechanism to put it at the front of the queue, and it
+      // won't cause any errors to execute it earlier than expected (just a bit of UI jank at worst)
+      // so we just continue happily along.
+      UIThreadOperation polledOperation = operationQueue.poll();
+      if (polledOperation == null) {
+        return;
+      }
+      polledOperation.execute(nodesManager);
     }
   }