Call applyReadyBatches synchronously from main thread from measure, o…

…therwise align to frame boundaries

Summary: By 1) always posting the runnable and 2) posting without using a choreographer, we were unnecessarily delaying notifying the RecyclerView about new data.

Reviewed By: pasqualeanatriello

Differential Revision: D14065346

fbshipit-source-id: d3fc20480cb0f39f6e3ea563e0c255b6c2ecc281

litho-it/src/test/java/com/facebook/litho/widget/RecyclerBinderTest.java

@@ -22,6 +22,7 @@
 import static com.facebook.litho.SizeSpec.makeSizeSpec;
 import static com.facebook.litho.widget.ComponentRenderInfo.create;
 import static org.assertj.core.api.Java6Assertions.assertThat;
+import static org.assertj.core.api.Java6Assertions.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.anyLong;
@@ -4735,6 +4736,80 @@ public boolean shouldUpdateLayoutHandler(
     assertThat(mHoldersForComponents.get(component).mLayoutHandler).isSameAs(layoutHandler);
   }
 
+  @Test
+  public void testDoNotApplyReadyBatchesWhileRecyclerViewIsInScrollOrLayout() {
+    ShadowLooper.pauseMainLooper();
+
+    final RecyclerBinder recyclerBinder =
+        new RecyclerBinder.Builder()
+            .layoutInfo(new LinearLayoutInfo(mComponentContext, OrientationHelper.VERTICAL, false))
+            .rangeRatio(10)
+            .build(mComponentContext);
+    final RecyclerView recyclerView = mock(RecyclerView.class);
+    when(recyclerView.isComputingLayout()).thenReturn(true);
+    recyclerBinder.mount(recyclerView);
+
+    final ArrayList<RenderInfo> renderInfos = new ArrayList<>();
+    for (int i = 0; i < 5; i++) {
+      final Component component =
+          TestDrawableComponent.create(mComponentContext).widthPx(100).heightPx(100).build();
+      renderInfos.add(ComponentRenderInfo.create().component(component).build());
+    }
+    recyclerBinder.measure(
+        new Size(), makeSizeSpec(200, EXACTLY), makeSizeSpec(200, EXACTLY), null);
+    recyclerBinder.insertRangeAtAsync(0, renderInfos);
+    recyclerBinder.notifyChangeSetCompleteAsync(true, NO_OP_CHANGE_SET_COMPLETE_CALLBACK);
+
+    mLayoutThreadShadowLooper.runToEndOfTasks();
+
+    // Run for a bit -- runTasks causes the test to hang because of how ShadowLooper is implemented
+    for (int i = 0; i < 10; i++) {
+      ShadowLooper.runMainLooperOneTask();
+    }
+
+    assertThat(recyclerBinder.getItemCount()).isEqualTo(0);
+
+    System.err.println("done");
+    when(recyclerView.isComputingLayout()).thenReturn(false);
+
+    ShadowLooper.runUiThreadTasks();
+
+    assertThat(recyclerBinder.getItemCount()).isEqualTo(5);
+  }
+
+  @Test(expected = RuntimeException.class)
+  public void testApplyReadyBatchesInfiniteLoop() {
+    ShadowLooper.pauseMainLooper();
+
+    final RecyclerBinder recyclerBinder =
+        new RecyclerBinder.Builder()
+            .layoutInfo(new LinearLayoutInfo(mComponentContext, OrientationHelper.VERTICAL, false))
+            .rangeRatio(10)
+            .build(mComponentContext);
+    final RecyclerView recyclerView = mock(RecyclerView.class);
+    when(recyclerView.isComputingLayout()).thenReturn(true);
+    recyclerBinder.mount(recyclerView);
+
+    final ArrayList<RenderInfo> renderInfos = new ArrayList<>();
+    for (int i = 0; i < 5; i++) {
+      final Component component =
+          TestDrawableComponent.create(mComponentContext).widthPx(100).heightPx(100).build();
+      renderInfos.add(ComponentRenderInfo.create().component(component).build());
+    }
+    recyclerBinder.measure(
+        new Size(), makeSizeSpec(200, EXACTLY), makeSizeSpec(200, EXACTLY), null);
+    recyclerBinder.insertRangeAtAsync(0, renderInfos);
+    recyclerBinder.notifyChangeSetCompleteAsync(true, NO_OP_CHANGE_SET_COMPLETE_CALLBACK);
+
+    mLayoutThreadShadowLooper.runToEndOfTasks();
+
+    for (int i = 0; i < 10000; i++) {
+      ShadowLooper.runMainLooperOneTask();
+    }
+
+    fail("Should have escaped infinite retries with exception.");
+  }
+
   private RecyclerBinder createRecyclerBinderWithMockAdapter(RecyclerView.Adapter adapterMock) {
     return new RecyclerBinder.Builder()
         .rangeRatio(RANGE_RATIO)

litho-widget/src/main/java/com/facebook/litho/widget/RecyclerBinder.java

@@ -60,6 +60,8 @@
 import com.facebook.litho.SizeSpec;
 import com.facebook.litho.ThreadPoolLayoutHandler;
 import com.facebook.litho.ThreadUtils;
+import com.facebook.litho.choreographercompat.ChoreographerCompat;
+import com.facebook.litho.choreographercompat.ChoreographerCompatImpl;
 import com.facebook.litho.config.ComponentsConfiguration;
 import com.facebook.litho.config.LayoutThreadPoolConfiguration;
 import com.facebook.litho.viewcompat.ViewBinder;
@@ -205,12 +207,7 @@ public void onSetRootAndSizeSpec(int width, int height) {
         @UiThread
         @Override
         public void onNewLayoutStateReady(ComponentTree componentTree) {
-          if (mMountedView == null) {
-            applyReadyBatches();
-          } else {
-            // When mounted, always apply binder mutations on frame boundaries
-            ViewCompat.postOnAnimation(mMountedView, mApplyReadyBatchesRunnable);
-          }
+          applyReadyBatches();
         }
       };
 
@@ -225,6 +222,17 @@ public void run() {
         }
       };
 
+  @VisibleForTesting
+  final ChoreographerCompat.FrameCallback mApplyReadyBatchesCallback =
+      new ChoreographerCompat.FrameCallback() {
+
+        @UiThread
+        @Override
+        public void doFrame(long frameTimeNanos) {
+          applyReadyBatches();
+        }
+      };
+
   private final boolean mIsCircular;
   private final boolean mHasDynamicItemHeight;
   private final boolean mWrapContent;
@@ -247,6 +255,7 @@ public void run() {
   private boolean mIsInitMounted = false; // Set to true when the first mount() is called.
   private @CommitPolicy int mCommitPolicy = CommitPolicy.IMMEDIATE;
   private boolean mHasFilledViewport = false;
+  private int mApplyReadyBatchesRetries = 0;
 
   @GuardedBy("this")
   private @Nullable AsyncBatch mCurrentBatch = null;
@@ -850,6 +859,14 @@ public final void insertRangeAtAsync(int position, List<RenderInfo> renderInfos)
     }
   }
 
+  private void ensureApplyReadyBatches() {
+    if (ThreadUtils.isMainThread()) {
+      applyReadyBatches();
+    } else {
+      ChoreographerCompatImpl.getInstance().postFrameCallback(mApplyReadyBatchesCallback);
+    }
+  }
+
   @UiThread
   private void applyReadyBatches() {
     ThreadUtils.assertMainThread();
@@ -862,9 +879,27 @@ private void applyReadyBatches() {
       // Fast check that doesn't acquire lock -- measure() is locking and will post a call to
       // applyReadyBatches when it completes.
       if (!mHasAsyncBatchesToCheck.get() || !mIsMeasured.get() || mIsInMeasure.get()) {
+        mApplyReadyBatchesRetries = 0;
+        return;
+      }
+
+      // If applyReadyBatches happens to be called from scroll of the RecyclerView (e.g. a scroll
+      // event triggers a new sections root synchronously which adds a component and calls
+      // applyReadyBatches), we need to postpone changing the adapter since RecyclerView asserts
+      // that changes don't happen while it's in scroll/layout.
+      if (mMountedView != null && mMountedView.isComputingLayout()) {
+        // Sanity check that we don't get stuck in an infinite loop
+        mApplyReadyBatchesRetries++;
+        if (mApplyReadyBatchesRetries > 100) {
+          throw new RuntimeException("Too many retries -- RecyclerView is stuck in layout.");
+        }
+
+        // Making changes to the adapter here will crash us. Just post to the next frame boundary.
+        ChoreographerCompatImpl.getInstance().postFrameCallback(mApplyReadyBatchesCallback);
         return;
       }
 
+      mApplyReadyBatchesRetries = 0;
       boolean appliedBatch = false;
       while (true) {
         final AsyncBatch batch;
@@ -1869,7 +1904,7 @@ public void measure(
     } finally {
       mIsInMeasure.set(false);
       if (mHasAsyncOperations) {
-        mMainThreadHandler.post(mApplyReadyBatchesRunnable);
+        ensureApplyReadyBatches();
       }
     }
   }