[Embedder] Only call removeview callback when raster thread is done with the view (flutter#164571)

Fixes
flutter#164564 (comment)

This would ensure that raster thread is completely done with the view,
i.e. it won't try to use the opengl context, which might be associated
with view window. So the client can know for sure, that when the
callback returns, it is safe to destroy the view and container window.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

---------

Co-authored-by: Loïc Sharma <[email protected]>

Author: knopp

engine/src/flutter/shell/common/shell.cc

@@ -2101,20 +2101,21 @@ void Shell::OnPlatformViewRemoveView(int64_t view_id,
        rasterizer = rasterizer_->GetWeakPtr(),  //
        view_id,                                 //
        callback = std::move(callback)           //
-  ] {
+  ]() mutable {
+        bool removed = false;
         if (engine) {
-          bool removed = engine->RemoveView(view_id);
-          callback(removed);
+          removed = engine->RemoveView(view_id);
         }
-        // Don't wait for the raster task here, which only cleans up memory and
-        // does not affect functionality. Make sure it is done after Dart
-        // removes the view to avoid receiving another rasterization request
-        // that adds back the view record.
-        task_runners.GetRasterTaskRunner()->PostTask([rasterizer, view_id]() {
-          if (rasterizer) {
-            rasterizer->CollectView(view_id);
-          }
-        });
+        task_runners.GetRasterTaskRunner()->PostTask(
+            [rasterizer, view_id, callback = std::move(callback), removed]() {
+              if (rasterizer) {
+                rasterizer->CollectView(view_id);
+              }
+              // Only call the callback after it is known for certain that the
+              // raster thread will not try to use resources associated with
+              // the view.
+              callback(removed);
+            });
       });
 }
 

engine/src/flutter/shell/platform/embedder/fixtures/main.dart

@@ -1511,6 +1511,12 @@ void window_metrics_event_all_view_ids() {
   signalNativeTest();
 }
 
+@pragma('vm:entry-point')
+// ignore: non_constant_identifier_names
+void remove_view_callback_too_early() {
+  signalNativeTest();
+}
+
 @pragma('vm:entry-point')
 // ignore: non_constant_identifier_names
 Future<void> channel_listener_response() async {

engine/src/flutter/shell/platform/embedder/tests/embedder_unittests.cc

@@ -1649,6 +1649,102 @@ TEST_F(EmbedderTest, CanRemoveView) {
   ASSERT_EQ(message, "View IDs: [0]");
 }
 
+// Regression test for:
+// https://github.com/flutter/flutter/issues/164564
+TEST_F(EmbedderTest, RemoveViewCallbackIsInvokedAfterRasterThreadIsDone) {
+  auto& context = GetEmbedderContext<EmbedderTestContextSoftware>();
+  EmbedderConfigBuilder builder(context);
+  std::mutex engine_mutex;
+  UniqueEngine engine;
+  auto render_thread = CreateNewThread("custom_render_thread");
+  EmbedderTestTaskRunner render_task_runner(
+      render_thread, [&](FlutterTask task) {
+        std::scoped_lock engine_lock(engine_mutex);
+        if (engine.is_valid()) {
+          ASSERT_EQ(FlutterEngineRunTask(engine.get(), &task), kSuccess);
+        }
+      });
+
+  builder.SetSurface(SkISize::Make(1, 1));
+  builder.SetDartEntrypoint("remove_view_callback_too_early");
+  builder.SetRenderTaskRunner(
+      &render_task_runner.GetFlutterTaskRunnerDescription());
+
+  fml::AutoResetWaitableEvent ready_latch;
+  context.AddNativeCallback(
+      "SignalNativeTest",
+      CREATE_NATIVE_ENTRY(
+          [&ready_latch](Dart_NativeArguments args) { ready_latch.Signal(); }));
+
+  {
+    std::scoped_lock lock(engine_mutex);
+    engine = builder.InitializeEngine();
+  }
+  ASSERT_EQ(FlutterEngineRunInitialized(engine.get()), kSuccess);
+  ASSERT_TRUE(engine.is_valid());
+
+  ready_latch.Wait();
+
+  fml::AutoResetWaitableEvent add_view_latch;
+  // Add view 123.
+  FlutterWindowMetricsEvent metrics = {};
+  metrics.struct_size = sizeof(FlutterWindowMetricsEvent);
+  metrics.width = 800;
+  metrics.height = 600;
+  metrics.pixel_ratio = 1.0;
+  metrics.view_id = 123;
+
+  FlutterAddViewInfo add_info = {};
+  add_info.struct_size = sizeof(FlutterAddViewInfo);
+  add_info.view_id = 123;
+  add_info.view_metrics = &metrics;
+  add_info.user_data = &add_view_latch;
+  add_info.add_view_callback = [](const FlutterAddViewResult* result) {
+    ASSERT_TRUE(result->added);
+    auto add_view_latch =
+        reinterpret_cast<fml::AutoResetWaitableEvent*>(result->user_data);
+    add_view_latch->Signal();
+  };
+  ASSERT_EQ(FlutterEngineAddView(engine.get(), &add_info), kSuccess);
+  add_view_latch.Wait();
+
+  std::atomic_bool view_available = true;
+
+  // Simulate pending rasterization task scheduled before view removal request
+  // that accesses view resources.
+  fml::AutoResetWaitableEvent raster_thread_latch;
+  render_thread->PostTask([&] {
+    std::this_thread::sleep_for(std::chrono::milliseconds(50));
+    // View must be available.
+    EXPECT_TRUE(view_available);
+    raster_thread_latch.Signal();
+  });
+
+  fml::AutoResetWaitableEvent remove_view_latch;
+  FlutterRemoveViewInfo remove_view_info = {};
+  remove_view_info.struct_size = sizeof(FlutterRemoveViewInfo);
+  remove_view_info.view_id = 123;
+  remove_view_info.user_data = &remove_view_latch;
+  remove_view_info.remove_view_callback =
+      [](const FlutterRemoveViewResult* result) {
+        ASSERT_TRUE(result->removed);
+        auto remove_view_latch =
+            reinterpret_cast<fml::AutoResetWaitableEvent*>(result->user_data);
+        remove_view_latch->Signal();
+      };
+
+  // Remove the view and wait until the callback is called.
+  ASSERT_EQ(FlutterEngineRemoveView(engine.get(), &remove_view_info), kSuccess);
+  remove_view_latch.Wait();
+
+  // After FlutterEngineRemoveViewCallback is called it should be safe to
+  // remove view - raster thread must not be accessing any view resources.
+  view_available = false;
+  raster_thread_latch.Wait();
+
+  FlutterEngineDeinitialize(engine.get());
+}
+
 //------------------------------------------------------------------------------
 /// The implicit view is a special view that the engine and framework assume
 /// can *always* be rendered to. Test that this view cannot be removed.