Reland: "Fix memory leak and bug in the RunsOnCreationTaskRun" (#25317)

Author: Emmanuel Garcia

fml/memory/task_runner_checker.cc

@@ -7,14 +7,18 @@
 namespace fml {
 
 TaskRunnerChecker::TaskRunnerChecker()
-    : initialized_queue_id_(InitTaskQueueId()){};
+    : initialized_queue_id_(InitTaskQueueId()),
+      subsumed_queue_id_(
+          MessageLoopTaskQueues::GetInstance()->GetSubsumedTaskQueueId(
+              initialized_queue_id_)){};
 
 TaskRunnerChecker::~TaskRunnerChecker() = default;
 
 bool TaskRunnerChecker::RunsOnCreationTaskRunner() const {
   FML_CHECK(fml::MessageLoop::IsInitializedForCurrentThread());
   const auto current_queue_id = MessageLoop::GetCurrentTaskQueueId();
-  return RunsOnTheSameThread(current_queue_id, initialized_queue_id_);
+  return RunsOnTheSameThread(current_queue_id, initialized_queue_id_) ||
+         RunsOnTheSameThread(current_queue_id, subsumed_queue_id_);
 };
 
 bool TaskRunnerChecker::RunsOnTheSameThread(TaskQueueId queue_a,

fml/memory/task_runner_checker.h

@@ -22,6 +22,7 @@ class TaskRunnerChecker final {
 
  private:
   TaskQueueId initialized_queue_id_;
+  TaskQueueId subsumed_queue_id_;
 
   TaskQueueId InitTaskQueueId();
 };

fml/memory/task_runner_checker_unittest.cc

@@ -115,5 +115,57 @@ TEST(TaskRunnerCheckerTests, MergedTaskRunnersRunsOnTheSameThread) {
   thread2.join();
 }
 
+TEST(TaskRunnerCheckerTests,
+     PassesRunsOnCreationTaskRunnerIfOnDifferentTaskRunner) {
+  fml::MessageLoop* loop1 = nullptr;
+  fml::AutoResetWaitableEvent latch1;
+  std::thread thread1([&]() {
+    fml::MessageLoop::EnsureInitializedForCurrentThread();
+    loop1 = &fml::MessageLoop::GetCurrent();
+    latch1.Signal();
+    loop1->Run();
+  });
+
+  fml::MessageLoop* loop2 = nullptr;
+  fml::AutoResetWaitableEvent latch2;
+  std::thread thread2([&]() {
+    fml::MessageLoop::EnsureInitializedForCurrentThread();
+    loop2 = &fml::MessageLoop::GetCurrent();
+    latch2.Signal();
+    loop2->Run();
+  });
+
+  latch1.Wait();
+  latch2.Wait();
+
+  fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId();
+  fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId();
+  fml::MessageLoopTaskQueues::GetInstance()->Merge(qid1, qid2);
+
+  std::unique_ptr<TaskRunnerChecker> checker;
+
+  fml::AutoResetWaitableEvent latch3;
+  loop2->GetTaskRunner()->PostTask([&]() {
+    checker = std::make_unique<TaskRunnerChecker>();
+    EXPECT_EQ(checker->RunsOnCreationTaskRunner(), true);
+    latch3.Signal();
+  });
+  latch3.Wait();
+
+  fml::MessageLoopTaskQueues::GetInstance()->Unmerge(qid1);
+
+  fml::AutoResetWaitableEvent latch4;
+  loop2->GetTaskRunner()->PostTask([&]() {
+    EXPECT_EQ(checker->RunsOnCreationTaskRunner(), true);
+    latch4.Signal();
+  });
+  latch4.Wait();
+
+  loop1->Terminate();
+  loop2->Terminate();
+  thread1.join();
+  thread2.join();
+}
+
 }  // namespace testing
 }  // namespace fml

fml/message_loop_task_queues.cc

@@ -238,7 +238,14 @@ bool MessageLoopTaskQueues::Unmerge(TaskQueueId owner) {
 bool MessageLoopTaskQueues::Owns(TaskQueueId owner,
                                  TaskQueueId subsumed) const {
   std::lock_guard guard(queue_mutex_);
-  return subsumed == queue_entries_.at(owner)->owner_of;
+  return owner != _kUnmerged && subsumed != _kUnmerged &&
+         subsumed == queue_entries_.at(owner)->owner_of;
+}
+
+TaskQueueId MessageLoopTaskQueues::GetSubsumedTaskQueueId(
+    TaskQueueId owner) const {
+  std::lock_guard guard(queue_mutex_);
+  return queue_entries_.at(owner)->owner_of;
 }
 
 // Subsumed queues will never have pending tasks.

fml/message_loop_task_queues.h

@@ -122,6 +122,10 @@ class MessageLoopTaskQueues
   // Returns true if owner owns the subsumed task queue.
   bool Owns(TaskQueueId owner, TaskQueueId subsumed) const;
 
+  // Returns the subsumed task queue if any or |TaskQueueId::kUnmerged|
+  // otherwise.
+  TaskQueueId GetSubsumedTaskQueueId(TaskQueueId owner) const;
+
  private:
   class MergedQueuesRunner;
 

fml/message_loop_task_queues_unittests.cc

@@ -185,6 +185,13 @@ TEST(MessageLoopTaskQueue, QueueDoNotOwnItself) {
   ASSERT_FALSE(task_queue->Owns(queue_id, queue_id));
 }
 
+TEST(MessageLoopTaskQueue, QueueDoNotOwnUnmergedTaskQueueId) {
+  auto task_queue = fml::MessageLoopTaskQueues::GetInstance();
+  ASSERT_FALSE(task_queue->Owns(task_queue->CreateTaskQueue(), _kUnmerged));
+  ASSERT_FALSE(task_queue->Owns(_kUnmerged, task_queue->CreateTaskQueue()));
+  ASSERT_FALSE(task_queue->Owns(_kUnmerged, _kUnmerged));
+}
+
 // TODO(chunhtai): This unit-test is flaky and sometimes fails asynchronizely
 // after the test has finished.
 // https://github.com/flutter/flutter/issues/43858

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

@@ -458,6 +458,7 @@ public void detach() {
     if (platformViewsChannel != null) {
       platformViewsChannel.setPlatformViewsHandler(null);
     }
+    destroyOverlaySurfaces();
     platformViewsChannel = null;
     context = null;
     textureRegistry = null;
@@ -485,6 +486,7 @@ public void attachToView(@NonNull View flutterView) {
    * the previously attached {@code View}.
    */
   public void detachFromView() {
+    destroyOverlaySurfaces();
     this.flutterView = null;
 
     // Inform all existing platform views that they are no longer associated with
@@ -703,6 +705,12 @@ private void initializeRootImageViewIfNeeded() {
     }
   }
 
+  /**
+   * Initializes a platform view and adds it to the view hierarchy.
+   *
+   * @param viewId The view ID.
+   * @hide
+   */
   @VisibleForTesting
   void initializePlatformViewIfNeeded(int viewId) {
     final PlatformView platformView = platformViews.get(viewId);
@@ -733,6 +741,19 @@ public void attachToFlutterRenderer(FlutterRenderer flutterRenderer) {
     androidTouchProcessor = new AndroidTouchProcessor(flutterRenderer, /*trackMotionEvents=*/ true);
   }
 
+  /**
+   * Called when a platform view id displayed in the current frame.
+   *
+   * @param viewId The ID of the platform view.
+   * @param x The left position relative to {@code FlutterView}.
+   * @param y The top position relative to {@code FlutterView}.
+   * @param width The width of the platform view.
+   * @param height The height of the platform view.
+   * @param viewWidth The original width of the platform view before applying the mutator stack.
+   * @param viewHeight The original height of the platform view before applying the mutator stack.
+   * @param mutatorsStack The mutator stack.
+   * @hide
+   */
   public void onDisplayPlatformView(
       int viewId,
       int x,
@@ -760,7 +781,20 @@ public void onDisplayPlatformView(
     currentFrameUsedPlatformViewIds.add(viewId);
   }
 
+  /**
+   * Called when an overlay surface is displayed in the current frame.
+   *
+   * @param id The ID of the surface.
+   * @param x The left position relative to {@code FlutterView}.
+   * @param y The top position relative to {@code FlutterView}.
+   * @param width The width of the surface.
+   * @param height The height of the surface.
+   * @hide
+   */
   public void onDisplayOverlaySurface(int id, int x, int y, int width, int height) {
+    if (overlayLayerViews.get(id) == null) {
+      throw new IllegalStateException("The overlay surface (id:" + id + ") doesn't exist");
+    }
     initializeRootImageViewIfNeeded();
 
     final FlutterImageView overlayView = overlayLayerViews.get(id);
@@ -782,6 +816,11 @@ public void onBeginFrame() {
     currentFrameUsedPlatformViewIds.clear();
   }
 
+  /**
+   * Called by {@code FlutterJNI} when the Flutter frame was submitted.
+   *
+   * @hide
+   */
   public void onEndFrame() {
     final FlutterView view = (FlutterView) flutterView;
     // If there are no platform views in the current frame,
@@ -850,6 +889,13 @@ private void finishFrame(boolean isFrameRenderedUsingImageReaders) {
     }
   }
 
+  /**
+   * Creates and tracks the overlay surface.
+   *
+   * @param imageView The surface that displays the overlay.
+   * @return Wrapper object that provides the layer id and the surface.
+   * @hide
+   */
   @VisibleForTesting
   @TargetApi(19)
   public FlutterOverlaySurface createOverlaySurface(@NonNull FlutterImageView imageView) {
@@ -858,6 +904,13 @@ public FlutterOverlaySurface createOverlaySurface(@NonNull FlutterImageView imag
     return new FlutterOverlaySurface(id, imageView.getSurface());
   }
 
+  /**
+   * Creates an overlay surface while the Flutter view is rendered by {@code FlutterImageView}.
+   *
+   * <p>This method is invoked by {@code FlutterJNI} only.
+   *
+   * @hide
+   */
   @TargetApi(19)
   public FlutterOverlaySurface createOverlaySurface() {
     // Overlay surfaces have the same size as the background surface.
@@ -874,12 +927,21 @@ public FlutterOverlaySurface createOverlaySurface() {
             FlutterImageView.SurfaceKind.overlay));
   }
 
+  /**
+   * Destroys the overlay surfaces and removes them from the view hierarchy.
+   *
+   * <p>This method is used only internally by {@code FlutterJNI}.
+   *
+   * @hide
+   */
   public void destroyOverlaySurfaces() {
     for (int i = 0; i < overlayLayerViews.size(); i++) {
       int overlayId = overlayLayerViews.keyAt(i);
       FlutterImageView overlayView = overlayLayerViews.valueAt(i);
       overlayView.detachFromRenderer();
-      ((FlutterView) flutterView).removeView(overlayView);
+      if (flutterView != null) {
+        ((FlutterView) flutterView).removeView(overlayView);
+      }
     }
     overlayLayerViews.clear();
   }

shell/platform/android/test/io/flutter/plugin/platform/PlatformViewsControllerTest.java

@@ -498,6 +498,131 @@ public void onEndFrame__removesPlatformViewParent() {
     assertEquals(flutterView.getChildCount(), 1);
   }
 
+  @Test
+  @Config(shadows = {ShadowFlutterSurfaceView.class, ShadowFlutterJNI.class})
+  public void detach__destroysOverlaySurfaces() {
+    final PlatformViewsController platformViewsController = new PlatformViewsController();
+
+    final int platformViewId = 0;
+    assertNull(platformViewsController.getPlatformViewById(platformViewId));
+
+    final PlatformViewFactory viewFactory = mock(PlatformViewFactory.class);
+    final PlatformView platformView = mock(PlatformView.class);
+    when(platformView.getView()).thenReturn(mock(View.class));
+    when(viewFactory.create(any(), eq(platformViewId), any())).thenReturn(platformView);
+
+    platformViewsController.getRegistry().registerViewFactory("testType", viewFactory);
+
+    final FlutterJNI jni = new FlutterJNI();
+    jni.attachToNative(false);
+    attach(jni, platformViewsController);
+
+    jni.onFirstFrame();
+
+    // Simulate create call from the framework.
+    createPlatformView(jni, platformViewsController, platformViewId, "testType");
+
+    // Produce a frame that displays a platform view and an overlay surface.
+    platformViewsController.onBeginFrame();
+    platformViewsController.onDisplayPlatformView(
+        platformViewId,
+        /* x=*/ 0,
+        /* y=*/ 0,
+        /* width=*/ 10,
+        /* height=*/ 10,
+        /* viewWidth=*/ 10,
+        /* viewHeight=*/ 10,
+        /* mutatorsStack=*/ new FlutterMutatorsStack());
+
+    final FlutterImageView overlayImageView = mock(FlutterImageView.class);
+    when(overlayImageView.acquireLatestImage()).thenReturn(true);
+
+    final FlutterOverlaySurface overlaySurface =
+        platformViewsController.createOverlaySurface(overlayImageView);
+    // This is OK.
+    platformViewsController.onDisplayOverlaySurface(
+        overlaySurface.getId(), /* x=*/ 0, /* y=*/ 0, /* width=*/ 10, /* height=*/ 10);
+
+    platformViewsController.detach();
+
+    assertThrows(
+        IllegalStateException.class,
+        () -> {
+          platformViewsController.onDisplayOverlaySurface(
+              overlaySurface.getId(), /* x=*/ 0, /* y=*/ 0, /* width=*/ 10, /* height=*/ 10);
+        });
+  }
+
+  @Test
+  @Config(shadows = {ShadowFlutterSurfaceView.class, ShadowFlutterJNI.class})
+  public void detachFromView__removesOverlaySurfaces() {
+    final PlatformViewsController platformViewsController = new PlatformViewsController();
+
+    final int platformViewId = 0;
+    assertNull(platformViewsController.getPlatformViewById(platformViewId));
+
+    final PlatformViewFactory viewFactory = mock(PlatformViewFactory.class);
+    final PlatformView platformView = mock(PlatformView.class);
+    when(platformView.getView()).thenReturn(mock(View.class));
+    when(viewFactory.create(any(), eq(platformViewId), any())).thenReturn(platformView);
+
+    platformViewsController.getRegistry().registerViewFactory("testType", viewFactory);
+
+    final FlutterJNI jni = new FlutterJNI();
+    jni.attachToNative(false);
+    attach(jni, platformViewsController);
+
+    final FlutterImageView overlayImageView = mock(FlutterImageView.class);
+    when(overlayImageView.acquireLatestImage()).thenReturn(true);
+
+    final FlutterOverlaySurface overlaySurface =
+        platformViewsController.createOverlaySurface(overlayImageView);
+
+    platformViewsController.onDisplayOverlaySurface(
+        overlaySurface.getId(), /* x=*/ 0, /* y=*/ 0, /* width=*/ 10, /* height=*/ 10);
+
+    platformViewsController.detachFromView();
+
+    assertThrows(
+        IllegalStateException.class,
+        () -> {
+          platformViewsController.onDisplayOverlaySurface(
+              overlaySurface.getId(), /* x=*/ 0, /* y=*/ 0, /* width=*/ 10, /* height=*/ 10);
+        });
+  }
+
+  @Test
+  @Config(shadows = {ShadowFlutterSurfaceView.class, ShadowFlutterJNI.class})
+  public void destroyOverlaySurfaces__doesNotThrowIfControllerIsDetached() {
+    final PlatformViewsController platformViewsController = new PlatformViewsController();
+
+    final int platformViewId = 0;
+    assertNull(platformViewsController.getPlatformViewById(platformViewId));
+
+    final PlatformViewFactory viewFactory = mock(PlatformViewFactory.class);
+    final PlatformView platformView = mock(PlatformView.class);
+    when(platformView.getView()).thenReturn(mock(View.class));
+    when(viewFactory.create(any(), eq(platformViewId), any())).thenReturn(platformView);
+
+    platformViewsController.getRegistry().registerViewFactory("testType", viewFactory);
+
+    final FlutterJNI jni = new FlutterJNI();
+    jni.attachToNative(false);
+    attach(jni, platformViewsController);
+
+    final FlutterImageView overlayImageView = mock(FlutterImageView.class);
+    when(overlayImageView.acquireLatestImage()).thenReturn(true);
+
+    final FlutterOverlaySurface overlaySurface =
+        platformViewsController.createOverlaySurface(overlayImageView);
+
+    platformViewsController.onDisplayOverlaySurface(
+        overlaySurface.getId(), /* x=*/ 0, /* y=*/ 0, /* width=*/ 10, /* height=*/ 10);
+
+    platformViewsController.detachFromView();
+    platformViewsController.destroyOverlaySurfaces();
+  }
+
   @Test
   public void checkInputConnectionProxy__falseIfViewIsNull() {
     final PlatformViewsController platformViewsController = new PlatformViewsController();