From 0cfb17c6fb33bea68dff61206ee68f44162f5bd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Sharma?= <737941+loic-sharma@users.noreply.github.com> Date: Wed, 12 Apr 2023 12:56:32 -0700 Subject: [PATCH] [Windows] Shutdown the engine before destroying the surface (flutter/engine#41012) [Windows] Shutdown the engine before destroying the surface --- .../platform/windows/angle_surface_manager.h | 7 +- .../platform/windows/flutter_windows_engine.h | 2 +- .../platform/windows/flutter_windows_view.cc | 6 ++ .../windows/flutter_windows_view_unittests.cc | 89 +++++++++++++++---- 4 files changed, 81 insertions(+), 23 deletions(-) diff --git a/engine/src/flutter/shell/platform/windows/angle_surface_manager.h b/engine/src/flutter/shell/platform/windows/angle_surface_manager.h index 85cbe2df99f..ca5ac61f7e9 100644 --- a/engine/src/flutter/shell/platform/windows/angle_surface_manager.h +++ b/engine/src/flutter/shell/platform/windows/angle_surface_manager.h @@ -28,7 +28,7 @@ namespace flutter { class AngleSurfaceManager { public: static std::unique_ptr Create(); - ~AngleSurfaceManager(); + virtual ~AngleSurfaceManager(); // Creates an EGLSurface wrapper and backing DirectX 11 SwapChain // associated with window, in the appropriate format for display. @@ -51,7 +51,7 @@ class AngleSurfaceManager { void GetSurfaceDimensions(EGLint* width, EGLint* height); // Releases the pass-in EGLSurface wrapping and backing resources if not null. - void DestroySurface(); + virtual void DestroySurface(); // Binds egl_context_ to the current rendering thread and to the draw and read // surfaces returning a boolean result reflecting success. @@ -79,11 +79,12 @@ class AngleSurfaceManager { // Gets the |ID3D11Device| chosen by ANGLE. bool GetDevice(ID3D11Device** device); - private: + protected: // Creates a new surface manager retaining reference to the passed-in target // for the lifetime of the manager. AngleSurfaceManager(); + private: bool Initialize(); void CleanUp(); diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h index a3e66d2c422..39ee739ed5a 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h @@ -109,7 +109,7 @@ class FlutterWindowsEngine { // Stops the engine. This invalidates the pointer returned by engine(). // // Returns false if stopping the engine fails, or if it was not running. - bool Stop(); + virtual bool Stop(); // Sets the view that is displaying this engine's content. void SetView(FlutterWindowsView* view); diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_view.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_view.cc index 0071c215125..2c3cca6e6aa 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_view.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_view.cc @@ -49,6 +49,12 @@ FlutterWindowsView::FlutterWindowsView( } FlutterWindowsView::~FlutterWindowsView() { + // The engine renders into the view's surface. The engine must be + // shutdown before the view's resources can be destroyed. + if (engine_) { + engine_->Stop(); + } + DestroyRenderSurface(); } diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_view_unittests.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_view_unittests.cc index 88444862a59..b56e51beec1 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_view_unittests.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_view_unittests.cc @@ -27,6 +27,9 @@ namespace flutter { namespace testing { +using ::testing::InSequence; +using ::testing::NiceMock; + constexpr uint64_t kScanCodeKeyA = 0x1e; constexpr uint64_t kVirtualKeyA = 0x41; @@ -55,16 +58,22 @@ std::unique_ptr> keyHandlingResponse(bool handled) { return flutter::JsonMessageCodec::GetInstance().EncodeMessage(document); } -// Returns an engine instance configured with dummy project path values, and -// overridden methods for sending platform messages, so that the engine can -// respond as if the framework were connected. -std::unique_ptr GetTestEngine() { +// Returns a Flutter project with the required path values to create +// a test engine. +FlutterProjectBundle GetTestProject() { FlutterDesktopEngineProperties properties = {}; properties.assets_path = L"C:\\foo\\flutter_assets"; properties.icu_data_path = L"C:\\foo\\icudtl.dat"; properties.aot_library_path = L"C:\\foo\\aot.so"; - FlutterProjectBundle project(properties); - auto engine = std::make_unique(project); + + return FlutterProjectBundle{properties}; +} + +// Returns an engine instance configured with test project path values, and +// overridden methods for sending platform messages, so that the engine can +// respond as if the framework were connected. +std::unique_ptr GetTestEngine() { + auto engine = std::make_unique(GetTestProject()); EngineModifier modifier(engine.get()); @@ -94,15 +103,57 @@ std::unique_ptr GetTestEngine() { return engine; } +class MockFlutterWindowsEngine : public FlutterWindowsEngine { + public: + MockFlutterWindowsEngine() : FlutterWindowsEngine(GetTestProject()) {} + + MOCK_METHOD0(Stop, bool()); + + private: + FML_DISALLOW_COPY_AND_ASSIGN(MockFlutterWindowsEngine); +}; + +class MockAngleSurfaceManager : public AngleSurfaceManager { + public: + MockAngleSurfaceManager() {} + + MOCK_METHOD0(DestroySurface, void()); + + private: + FML_DISALLOW_COPY_AND_ASSIGN(MockAngleSurfaceManager); +}; + } // namespace +// The view's surface must be destroyed after the engine is shutdown. +// See: https://github.com/flutter/flutter/issues/124463 +TEST(FlutterWindowsViewTest, Shutdown) { + std::unique_ptr engine = + std::make_unique(); + auto window_binding_handler = + std::make_unique>(); + std::unique_ptr surface_manager = + std::make_unique(); + + EngineModifier modifier(engine.get()); + FlutterWindowsView view(std::move(window_binding_handler)); + + // The engine must be stopped before the surface can be destroyed. + InSequence s; + EXPECT_CALL(*engine.get(), Stop).Times(1); + EXPECT_CALL(*surface_manager.get(), DestroySurface).Times(1); + + modifier.SetSurfaceManager(surface_manager.release()); + view.SetEngine(std::move(engine)); +} + TEST(FlutterWindowsViewTest, KeySequence) { std::unique_ptr engine = GetTestEngine(); test_response = false; auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -130,7 +181,7 @@ TEST(FlutterWindowsViewTest, EnableSemantics) { }); auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -138,7 +189,7 @@ TEST(FlutterWindowsViewTest, EnableSemantics) { EXPECT_TRUE(semantics_enabled); } -TEST(FlutterWindowsView, AddSemanticsNodeUpdate) { +TEST(FlutterWindowsViewTest, AddSemanticsNodeUpdate) { std::unique_ptr engine = GetTestEngine(); EngineModifier modifier(engine.get()); modifier.embedder_api().UpdateSemanticsEnabled = @@ -147,7 +198,7 @@ TEST(FlutterWindowsView, AddSemanticsNodeUpdate) { }; auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -237,7 +288,7 @@ TEST(FlutterWindowsView, AddSemanticsNodeUpdate) { // node3 // // node0 and node2 are grouping nodes. node1 and node2 are static text nodes. -TEST(FlutterWindowsView, AddSemanticsNodeUpdateWithChildren) { +TEST(FlutterWindowsViewTest, AddSemanticsNodeUpdateWithChildren) { std::unique_ptr engine = GetTestEngine(); EngineModifier modifier(engine.get()); modifier.embedder_api().UpdateSemanticsEnabled = @@ -246,7 +297,7 @@ TEST(FlutterWindowsView, AddSemanticsNodeUpdateWithChildren) { }; auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -435,7 +486,7 @@ TEST(FlutterWindowsView, AddSemanticsNodeUpdateWithChildren) { // node2 // // node1 is a grouping node, node0 is a static text node. -TEST(FlutterWindowsView, NonZeroSemanticsRoot) { +TEST(FlutterWindowsViewTest, NonZeroSemanticsRoot) { std::unique_ptr engine = GetTestEngine(); EngineModifier modifier(engine.get()); modifier.embedder_api().UpdateSemanticsEnabled = @@ -444,7 +495,7 @@ TEST(FlutterWindowsView, NonZeroSemanticsRoot) { }; auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -576,7 +627,7 @@ TEST(FlutterWindowsViewTest, AccessibilityHitTesting) { }; auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -662,7 +713,7 @@ TEST(FlutterWindowsViewTest, WindowResizeTests) { EngineModifier modifier(engine.get()); auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -721,7 +772,7 @@ TEST(FlutterWindowsViewTest, CheckboxNativeState) { }; auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -867,7 +918,7 @@ TEST(FlutterWindowsViewTest, SwitchNativeState) { }; auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine)); @@ -984,7 +1035,7 @@ TEST(FlutterWindowsViewTest, TooltipNodeData) { }; auto window_binding_handler = - std::make_unique<::testing::NiceMock>(); + std::make_unique>(); FlutterWindowsView view(std::move(window_binding_handler)); view.SetEngine(std::move(engine));