From cdaa61208e21a6ce61298f97bba40fcdd7946b95 Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Fri, 29 Mar 2024 09:59:01 -0700 Subject: [PATCH 1/2] [Windows] Fix EGL surface destruction race --- .../platform/windows/flutter_windows_view.cc | 31 ++++++++++--- shell/platform/windows/flutter_windows_view.h | 3 -- .../windows/flutter_windows_view_unittests.cc | 45 +++++++++++++++++++ 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/shell/platform/windows/flutter_windows_view.cc b/shell/platform/windows/flutter_windows_view.cc index 815cbaa3bb4f8..51c55d31db3ab 100644 --- a/shell/platform/windows/flutter_windows_view.cc +++ b/shell/platform/windows/flutter_windows_view.cc @@ -8,6 +8,7 @@ #include "flutter/common/constants.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 +82,26 @@ void UpdateVsync(const FlutterWindowsEngine& engine, } } +/// Destroys a rendering surface that backs a Flutter view. +void DestroyWindowSurface(const FlutterWindowsEngine& engine, + 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()) { + fml::AutoResetWaitableEvent latch; + engine.PostRasterThreadTask([&]() { + surface.Destroy(); + latch.Signal(); + }); + latch.Wait(); + } 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 +126,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_, *surface_); + } } bool FlutterWindowsView::OnEmptyFrameGenerated() { @@ -706,12 +729,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(); } diff --git a/shell/platform/windows/flutter_windows_view.h b/shell/platform/windows/flutter_windows_view.h index 8cd4e5e71312b..782e63ad53136 100644 --- a/shell/platform/windows/flutter_windows_view.h +++ b/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. diff --git a/shell/platform/windows/flutter_windows_view_unittests.cc b/shell/platform/windows/flutter_windows_view_unittests.cc index b4bb9a4789d90..c22459f32bd3d 100644 --- a/shell/platform/windows/flutter_windows_view_unittests.cc +++ b/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>(); std::unique_ptr 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(); auto surface = std::make_unique(); @@ -883,7 +896,14 @@ TEST(FlutterWindowsViewTest, TestEmptyFrameResizes) { auto windows_proc_table = std::make_shared>(); std::unique_ptr 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(); auto surface = std::make_unique(); @@ -940,7 +960,14 @@ TEST(FlutterWindowsViewTest, TestEmptyFrameResizes) { // https://github.com/flutter/flutter/issues/141855 TEST(FlutterWindowsViewTest, WindowResizeRace) { std::unique_ptr 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(); auto surface = std::make_unique(); @@ -980,7 +1007,14 @@ TEST(FlutterWindowsViewTest, WindowResizeRace) { // even though EGL initialized successfully. TEST(FlutterWindowsViewTest, WindowResizeInvalidSurface) { std::unique_ptr 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(); auto surface = std::make_unique(); @@ -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()}; From cdf35c602385085a38606e95f2c1ba2eb13bdb7f Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Fri, 29 Mar 2024 14:05:52 -0700 Subject: [PATCH 2/2] Remove latch --- shell/platform/windows/flutter_windows_view.cc | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/shell/platform/windows/flutter_windows_view.cc b/shell/platform/windows/flutter_windows_view.cc index 51c55d31db3ab..93f523194714a 100644 --- a/shell/platform/windows/flutter_windows_view.cc +++ b/shell/platform/windows/flutter_windows_view.cc @@ -7,6 +7,7 @@ #include #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" @@ -84,21 +85,17 @@ void UpdateVsync(const FlutterWindowsEngine& engine, /// Destroys a rendering surface that backs a Flutter view. void DestroyWindowSurface(const FlutterWindowsEngine& engine, - egl::WindowSurface& surface) { + std::unique_ptr 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()) { - fml::AutoResetWaitableEvent latch; - engine.PostRasterThreadTask([&]() { - surface.Destroy(); - latch.Signal(); - }); - latch.Wait(); + 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(); + surface->Destroy(); } } @@ -127,7 +124,7 @@ FlutterWindowsView::~FlutterWindowsView() { engine_->OnWindowStateEvent(GetWindowHandle(), WindowStateEvent::kHide); if (surface_) { - DestroyWindowSurface(*engine_, *surface_); + DestroyWindowSurface(*engine_, std::move(surface_)); } }