From e0b8175ef42d07db81c93cb56ab16fbc56487bcb Mon Sep 17 00:00:00 2001 From: yaakovschectman <109111084+yaakovschectman@users.noreply.github.com> Date: Wed, 10 May 2023 07:55:39 -0400 Subject: [PATCH] Only register top level window message listener upon registering service binding (flutter/engine#41733) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move registering the `WM_CLOSE` message listener in the engine from initialization to in response to a message sent from `ServiceBinding` upon its initialization so that we do not intercept window messages when there is no binding registered. Addresses https://github.com/flutter/flutter/issues/126033. ## 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] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> --- .../macos/framework/Source/FlutterEngine.mm | 4 ++ .../platform/linux/fl_platform_plugin.cc | 6 +++ .../windows/flutter_windows_engine.cc | 4 ++ .../platform/windows/flutter_windows_engine.h | 4 ++ .../flutter_windows_engine_unittests.cc | 46 +++++++++++++++++++ .../platform/windows/platform_handler.cc | 5 ++ .../windows/windows_lifecycle_manager.cc | 9 +++- .../windows/windows_lifecycle_manager.h | 5 ++ 8 files changed, 82 insertions(+), 1 deletion(-) diff --git a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm index 16e8405a6f..91aa17e903 100644 --- a/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm +++ b/engine/src/flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm @@ -1031,6 +1031,10 @@ static void OnPlatformMessage(const FlutterPlatformMessage* message, FlutterEngi result(@{@"value" : @([self clipboardHasStrings])}); } else if ([call.method isEqualToString:@"System.exitApplication"]) { [[self terminationHandler] handleRequestAppExitMethodCall:call.arguments result:result]; + } else if ([call.method isEqualToString:@"System.initializationComplete"]) { + // TODO(gspencergoog): Handle this message to enable exit message listening. + // https://github.com/flutter/flutter/issues/126033 + result(nil); } else { result(FlutterMethodNotImplemented); } diff --git a/engine/src/flutter/shell/platform/linux/fl_platform_plugin.cc b/engine/src/flutter/shell/platform/linux/fl_platform_plugin.cc index 2a2065bbb3..35b954f6b1 100644 --- a/engine/src/flutter/shell/platform/linux/fl_platform_plugin.cc +++ b/engine/src/flutter/shell/platform/linux/fl_platform_plugin.cc @@ -20,6 +20,8 @@ static constexpr char kSetClipboardDataMethod[] = "Clipboard.setData"; static constexpr char kClipboardHasStringsMethod[] = "Clipboard.hasStrings"; static constexpr char kExitApplicationMethod[] = "System.exitApplication"; static constexpr char kRequestAppExitMethod[] = "System.requestAppExit"; +static constexpr char kInitializationCompleteMethod[] = + "System.initializationComplete"; static constexpr char kPlaySoundMethod[] = "SystemSound.play"; static constexpr char kSystemNavigatorPopMethod[] = "SystemNavigator.pop"; static constexpr char kTextKey[] = "text"; @@ -335,6 +337,10 @@ static void method_call_cb(FlMethodChannel* channel, response = system_sound_play(self, args); } else if (strcmp(method, kSystemNavigatorPopMethod) == 0) { response = system_navigator_pop(self); + } else if (strcmp(method, kInitializationCompleteMethod) == 0) { + // TODO(gspencergoog): Handle this message to enable exit message listening. + // https://github.com/flutter/flutter/issues/126033 + response = FL_METHOD_RESPONSE(fl_method_success_response_new(nullptr)); } else { response = FL_METHOD_RESPONSE(fl_method_not_implemented_response_new()); } diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc index 39dbe8ea6b..8d8f3f15d3 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.cc @@ -778,4 +778,8 @@ void FlutterWindowsEngine::OnDwmCompositionChanged() { view_->OnDwmCompositionChanged(); } +void FlutterWindowsEngine::OnApplicationLifecycleEnabled() { + lifecycle_manager_->BeginProcessingClose(); +} + } // namespace flutter 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 797cc2142d..36d21f822d 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_engine.h @@ -269,6 +269,10 @@ class FlutterWindowsEngine { // Called when a WM_DWMCOMPOSITIONCHANGED message is received. void OnDwmCompositionChanged(); + // Called in response to the framework registering a ServiceBindings. + // Registers the top level handler for the WM_CLOSE window message. + void OnApplicationLifecycleEnabled(); + protected: // Creates the keyboard key handler. // diff --git a/engine/src/flutter/shell/platform/windows/flutter_windows_engine_unittests.cc b/engine/src/flutter/shell/platform/windows/flutter_windows_engine_unittests.cc index 161181cd11..d518e7f9c1 100644 --- a/engine/src/flutter/shell/platform/windows/flutter_windows_engine_unittests.cc +++ b/engine/src/flutter/shell/platform/windows/flutter_windows_engine_unittests.cc @@ -657,6 +657,7 @@ TEST_F(FlutterWindowsEngineTest, TestExit) { ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([]() { return true; }); EXPECT_CALL(*handler, Quit).Times(1); modifier.SetLifecycleManager(std::move(handler)); + engine->OnApplicationLifecycleEnabled(); engine->Run(); @@ -693,6 +694,7 @@ TEST_F(FlutterWindowsEngineTest, TestExitCancel) { ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([]() { return true; }); EXPECT_CALL(*handler, Quit).Times(0); modifier.SetLifecycleManager(std::move(handler)); + engine->OnApplicationLifecycleEnabled(); auto binary_messenger = std::make_unique(engine->messenger()); @@ -753,6 +755,7 @@ TEST_F(FlutterWindowsEngineTest, TestExitSecondCloseMessage) { hwnd, msg, wparam, lparam); }); modifier.SetLifecycleManager(std::move(handler)); + engine->OnApplicationLifecycleEnabled(); engine->Run(); @@ -803,6 +806,7 @@ TEST_F(FlutterWindowsEngineTest, TestExitCloseMultiWindow) { // Quit should not be called when there is more than one window. EXPECT_CALL(*handler, Quit).Times(0); modifier.SetLifecycleManager(std::move(handler)); + engine->OnApplicationLifecycleEnabled(); engine->Run(); @@ -814,5 +818,47 @@ TEST_F(FlutterWindowsEngineTest, TestExitCloseMultiWindow) { } } +TEST_F(FlutterWindowsEngineTest, LifecycleManagerDisabledByDefault) { + FlutterWindowsEngineBuilder builder{GetContext()}; + + auto window_binding_handler = + std::make_unique<::testing::NiceMock>(); + MockFlutterWindowsView view(std::move(window_binding_handler)); + view.SetEngine(builder.Build()); + FlutterWindowsEngine* engine = view.GetEngine(); + + EngineModifier modifier(engine); + modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; }; + auto handler = std::make_unique(engine); + EXPECT_CALL(*handler, IsLastWindowOfProcess).Times(0); + modifier.SetLifecycleManager(std::move(handler)); + + engine->window_proc_delegate_manager()->OnTopLevelWindowProc(0, WM_CLOSE, 0, + 0); +} + +TEST_F(FlutterWindowsEngineTest, EnableApplicationLifecycle) { + FlutterWindowsEngineBuilder builder{GetContext()}; + + auto window_binding_handler = + std::make_unique<::testing::NiceMock>(); + MockFlutterWindowsView view(std::move(window_binding_handler)); + view.SetEngine(builder.Build()); + FlutterWindowsEngine* engine = view.GetEngine(); + + EngineModifier modifier(engine); + modifier.embedder_api().RunsAOTCompiledDartCode = []() { return false; }; + auto handler = std::make_unique(engine); + ON_CALL(*handler, IsLastWindowOfProcess).WillByDefault([]() { + return false; + }); + EXPECT_CALL(*handler, IsLastWindowOfProcess).Times(1); + modifier.SetLifecycleManager(std::move(handler)); + engine->OnApplicationLifecycleEnabled(); + + engine->window_proc_delegate_manager()->OnTopLevelWindowProc(0, WM_CLOSE, 0, + 0); +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/shell/platform/windows/platform_handler.cc b/engine/src/flutter/shell/platform/windows/platform_handler.cc index ab9a79a6a0..771d3d56b9 100644 --- a/engine/src/flutter/shell/platform/windows/platform_handler.cc +++ b/engine/src/flutter/shell/platform/windows/platform_handler.cc @@ -23,6 +23,8 @@ static constexpr char kHasStringsClipboardMethod[] = "Clipboard.hasStrings"; static constexpr char kSetClipboardDataMethod[] = "Clipboard.setData"; static constexpr char kExitApplicationMethod[] = "System.exitApplication"; static constexpr char kRequestAppExitMethod[] = "System.requestAppExit"; +static constexpr char kInitializationCompleteMethod[] = + "System.initializationComplete"; static constexpr char kPlaySoundMethod[] = "SystemSound.play"; static constexpr char kExitCodeKey[] = "exitCode"; @@ -495,6 +497,9 @@ void PlatformHandler::HandleMethodCall( const rapidjson::Value& sound_type = method_call.arguments()[0]; SystemSoundPlay(sound_type.GetString(), std::move(result)); + } else if (method.compare(kInitializationCompleteMethod) == 0) { + engine_->OnApplicationLifecycleEnabled(); + result->Success(); } else { result->NotImplemented(); } diff --git a/engine/src/flutter/shell/platform/windows/windows_lifecycle_manager.cc b/engine/src/flutter/shell/platform/windows/windows_lifecycle_manager.cc index c7c46fa1e9..f8bf5de37f 100644 --- a/engine/src/flutter/shell/platform/windows/windows_lifecycle_manager.cc +++ b/engine/src/flutter/shell/platform/windows/windows_lifecycle_manager.cc @@ -14,7 +14,7 @@ namespace flutter { WindowsLifecycleManager::WindowsLifecycleManager(FlutterWindowsEngine* engine) - : engine_(engine) {} + : engine_(engine), process_close_(false) {} WindowsLifecycleManager::~WindowsLifecycleManager() {} @@ -49,6 +49,9 @@ bool WindowsLifecycleManager::WindowProc(HWND hwnd, // is, we re-dispatch a new WM_CLOSE message. In order to allow the new // message to reach other delegates, we ignore it here. case WM_CLOSE: { + if (!process_close_) { + return false; + } auto key = std::make_tuple(hwnd, wpar, lpar); auto itr = sent_close_messages_.find(key); if (itr != sent_close_messages_.end()) { @@ -156,4 +159,8 @@ bool WindowsLifecycleManager::IsLastWindowOfProcess() { return num_windows <= 1; } +void WindowsLifecycleManager::BeginProcessingClose() { + process_close_ = true; +} + } // namespace flutter diff --git a/engine/src/flutter/shell/platform/windows/windows_lifecycle_manager.h b/engine/src/flutter/shell/platform/windows/windows_lifecycle_manager.h index e1d1f604c4..b08ec57a4d 100644 --- a/engine/src/flutter/shell/platform/windows/windows_lifecycle_manager.h +++ b/engine/src/flutter/shell/platform/windows/windows_lifecycle_manager.h @@ -37,6 +37,9 @@ class WindowsLifecycleManager { // Intercept top level window messages, only paying attention to WM_CLOSE. bool WindowProc(HWND hwnd, UINT msg, WPARAM w, LPARAM l, LRESULT* result); + // Signal to start consuming WM_CLOSE messages. + void BeginProcessingClose(); + protected: // Check the number of top-level windows associated with this process, and // return true only if there are 1 or fewer. @@ -51,6 +54,8 @@ class WindowsLifecycleManager { FlutterWindowsEngine* engine_; std::map, int> sent_close_messages_; + + bool process_close_; }; } // namespace flutter