[Windows] Fix EGL surface destruction race (flutter#51781)

This fixes the `WindowsTest.EngineCanTransitionToHeadless` flakiness reported by @matanlurey.

EGL surfaces can only be used by a single thread at a time. Concurrent operations are unsafe.

Previously, the EGL surface was destroyed on the platform thread. This was safe as this always happened after the engine was shutdown and the raster thread was stopped. However, in a multi-view world a view can be destroyed while the engine is running. There may be pending raster tasks that operate on the render surface. Thus, the EGL surfaces should be destroyed on the raster thread when it is available.

This bug was introduced by flutter/engine#51681
Part of flutter#142845

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Author: loic-sharma

shell/platform/windows/flutter_windows_view.cc

@@ -7,7 +7,9 @@
 #include <chrono>
 
 #include "flutter/common/constants.h"
+#include "flutter/fml/make_copyable.h"
 #include "flutter/fml/platform/win/wstring_conversion.h"
+#include "flutter/fml/synchronization/waitable_event.h"
 #include "flutter/shell/platform/common/accessibility_bridge.h"
 #include "flutter/shell/platform/windows/keyboard_key_channel_handler.h"
 #include "flutter/shell/platform/windows/text_input_plugin.h"
@@ -81,6 +83,22 @@ void UpdateVsync(const FlutterWindowsEngine& engine,
   }
 }
 
+/// Destroys a rendering surface that backs a Flutter view.
+void DestroyWindowSurface(const FlutterWindowsEngine& engine,
+                          std::unique_ptr<egl::WindowSurface> surface) {
+  // EGL surfaces are used on the raster thread if the engine is running.
+  // There may be pending raster tasks that use this surface. Destroy the
+  // surface on the raster thread to avoid concurrent uses.
+  if (engine.running()) {
+    engine.PostRasterThreadTask(fml::MakeCopyable(
+        [surface = std::move(surface)] { surface->Destroy(); }));
+  } else {
+    // There's no raster thread if engine isn't running. The surface can be
+    // destroyed on the platform thread.
+    surface->Destroy();
+  }
+}
+
 }  // namespace
 
 FlutterWindowsView::FlutterWindowsView(
@@ -105,7 +123,9 @@ FlutterWindowsView::~FlutterWindowsView() {
   // Notify the engine the view's child window will no longer be visible.
   engine_->OnWindowStateEvent(GetWindowHandle(), WindowStateEvent::kHide);
 
-  DestroyRenderSurface();
+  if (surface_) {
+    DestroyWindowSurface(*engine_, std::move(surface_));
+  }
 }
 
 bool FlutterWindowsView::OnEmptyFrameGenerated() {
@@ -706,12 +726,6 @@ bool FlutterWindowsView::ResizeRenderSurface(size_t width, size_t height) {
   return true;
 }
 
-void FlutterWindowsView::DestroyRenderSurface() {
-  if (surface_) {
-    surface_->Destroy();
-  }
-}
-
 egl::WindowSurface* FlutterWindowsView::surface() const {
   return surface_.get();
 }

shell/platform/windows/flutter_windows_view.h

@@ -50,9 +50,6 @@ class FlutterWindowsView : public WindowBindingHandlerDelegate {
   // Should be called before calling FlutterEngineRun using this view.
   void CreateRenderSurface();
 
-  // Destroys current rendering surface if one has been allocated.
-  void DestroyRenderSurface();
-
   // Get the EGL surface that backs the Flutter view.
   //
   // This might be nullptr or an invalid surface.

shell/platform/windows/flutter_windows_view_unittests.cc

@@ -264,6 +264,12 @@ TEST(FlutterWindowsViewTest, Shutdown) {
     InSequence s;
     EXPECT_CALL(*engine_ptr, running).WillOnce(Return(true));
     EXPECT_CALL(*engine_ptr, RemoveView(view_id)).Times(1);
+    EXPECT_CALL(*engine_ptr, running).WillOnce(Return(true));
+    EXPECT_CALL(*engine_ptr, PostRasterThreadTask)
+        .WillOnce([](fml::closure callback) {
+          callback();
+          return true;
+        });
     EXPECT_CALL(*surface_ptr, Destroy).Times(1);
   }
 }
@@ -825,7 +831,14 @@ TEST(FlutterWindowsViewTest, WindowResizeTests) {
   auto windows_proc_table = std::make_shared<NiceMock<MockWindowsProcTable>>();
   std::unique_ptr<FlutterWindowsEngine> engine =
       GetTestEngine(windows_proc_table);
+
   EngineModifier engine_modifier{engine.get()};
+  engine_modifier.embedder_api().PostRenderThreadTask = MOCK_ENGINE_PROC(
+      PostRenderThreadTask,
+      ([](auto engine, VoidCallback callback, void* user_data) {
+        callback(user_data);
+        return kSuccess;
+      }));
 
   auto egl_manager = std::make_unique<egl::MockManager>();
   auto surface = std::make_unique<egl::MockWindowSurface>();
@@ -883,7 +896,14 @@ TEST(FlutterWindowsViewTest, TestEmptyFrameResizes) {
   auto windows_proc_table = std::make_shared<NiceMock<MockWindowsProcTable>>();
   std::unique_ptr<FlutterWindowsEngine> engine =
       GetTestEngine(windows_proc_table);
+
   EngineModifier engine_modifier{engine.get()};
+  engine_modifier.embedder_api().PostRenderThreadTask = MOCK_ENGINE_PROC(
+      PostRenderThreadTask,
+      ([](auto engine, VoidCallback callback, void* user_data) {
+        callback(user_data);
+        return kSuccess;
+      }));
 
   auto egl_manager = std::make_unique<egl::MockManager>();
   auto surface = std::make_unique<egl::MockWindowSurface>();
@@ -940,7 +960,14 @@ TEST(FlutterWindowsViewTest, TestEmptyFrameResizes) {
 // https://github.com/flutter/flutter/issues/141855
 TEST(FlutterWindowsViewTest, WindowResizeRace) {
   std::unique_ptr<FlutterWindowsEngine> engine = GetTestEngine();
+
   EngineModifier engine_modifier(engine.get());
+  engine_modifier.embedder_api().PostRenderThreadTask = MOCK_ENGINE_PROC(
+      PostRenderThreadTask,
+      ([](auto engine, VoidCallback callback, void* user_data) {
+        callback(user_data);
+        return kSuccess;
+      }));
 
   auto egl_manager = std::make_unique<egl::MockManager>();
   auto surface = std::make_unique<egl::MockWindowSurface>();
@@ -980,7 +1007,14 @@ TEST(FlutterWindowsViewTest, WindowResizeRace) {
 // even though EGL initialized successfully.
 TEST(FlutterWindowsViewTest, WindowResizeInvalidSurface) {
   std::unique_ptr<FlutterWindowsEngine> engine = GetTestEngine();
+
   EngineModifier engine_modifier(engine.get());
+  engine_modifier.embedder_api().PostRenderThreadTask = MOCK_ENGINE_PROC(
+      PostRenderThreadTask,
+      ([](auto engine, VoidCallback callback, void* user_data) {
+        callback(user_data);
+        return kSuccess;
+      }));
 
   auto egl_manager = std::make_unique<egl::MockManager>();
   auto surface = std::make_unique<egl::MockWindowSurface>();
@@ -1477,6 +1511,11 @@ TEST(FlutterWindowsViewTest, DisablesVSyncAfterStartup) {
   EXPECT_CALL(*surface_ptr, MakeCurrent).WillOnce(Return(true));
   EXPECT_CALL(*surface_ptr, SetVSyncEnabled(false)).WillOnce(Return(true));
   EXPECT_CALL(render_context, ClearCurrent).WillOnce(Return(true));
+  EXPECT_CALL(*engine.get(), PostRasterThreadTask)
+      .WillOnce([](fml::closure callback) {
+        callback();
+        return true;
+      });
   EXPECT_CALL(*surface_ptr, Destroy).Times(1);
 
   EngineModifier modifier{engine.get()};
@@ -1519,6 +1558,12 @@ TEST(FlutterWindowsViewTest, EnablesVSyncAfterStartup) {
   EXPECT_CALL(*surface_ptr, MakeCurrent).WillOnce(Return(true));
   EXPECT_CALL(*surface_ptr, SetVSyncEnabled(true)).WillOnce(Return(true));
   EXPECT_CALL(render_context, ClearCurrent).WillOnce(Return(true));
+
+  EXPECT_CALL(*engine.get(), PostRasterThreadTask)
+      .WillOnce([](fml::closure callback) {
+        callback();
+        return true;
+      });
   EXPECT_CALL(*surface_ptr, Destroy).Times(1);
 
   EngineModifier modifier{engine.get()};