From faf44fed5a5913dcbeebd7ead8e3933a5e72a6fc Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Mon, 6 Apr 2020 09:55:42 -0700 Subject: [PATCH] Improve C++ plugin lifetime handling (#17489) This makes two changes: - Adds a way to register a callback for when a FlutterDesktopPluginRegistrarRef is destroyed, and implements the logic to call it in the Windows and Linux embeddings. - Adds a class to the C++ wrapper that handles making a singleton owning PluginRegistrar wrappers, and destroying them when the underlying reference goes away, to avoid needing that boilerplate code in every plugin's source. Fixes https://github.com/flutter/flutter/issues/53496 --- .../common/cpp/client_wrapper/BUILD.gn | 2 + .../include/flutter/plugin_registrar.h | 43 +++++++++++ .../cpp/client_wrapper/plugin_registrar.cc | 18 ++++- .../plugin_registrar_unittests.cc | 75 ++++++++++++++++--- .../testing/stub_flutter_api.cc | 8 ++ .../client_wrapper/testing/stub_flutter_api.h | 4 + .../cpp/public/flutter_plugin_registrar.h | 9 +++ shell/platform/glfw/flutter_glfw.cc | 14 ++++ shell/platform/windows/flutter_windows.cc | 6 ++ .../platform/windows/win32_flutter_window.cc | 3 + shell/platform/windows/window_state.h | 3 + 11 files changed, 175 insertions(+), 10 deletions(-) diff --git a/shell/platform/common/cpp/client_wrapper/BUILD.gn b/shell/platform/common/cpp/client_wrapper/BUILD.gn index f46cd8146..ed0bdf9e8 100644 --- a/shell/platform/common/cpp/client_wrapper/BUILD.gn +++ b/shell/platform/common/cpp/client_wrapper/BUILD.gn @@ -65,4 +65,6 @@ executable("client_wrapper_unittests") { # https://github.com/flutter/flutter/issues/41414. "//third_party/dart/runtime:libdart_jit", ] + + defines = [ "FLUTTER_DESKTOP_LIBRARY" ] } diff --git a/shell/platform/common/cpp/client_wrapper/include/flutter/plugin_registrar.h b/shell/platform/common/cpp/client_wrapper/include/flutter/plugin_registrar.h index 5fbe1b5b6..6169d6437 100644 --- a/shell/platform/common/cpp/client_wrapper/include/flutter/plugin_registrar.h +++ b/shell/platform/common/cpp/client_wrapper/include/flutter/plugin_registrar.h @@ -5,6 +5,7 @@ #ifndef FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_PLUGIN_REGISTRAR_H_ #define FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_PLUGIN_REGISTRAR_H_ +#include #include #include #include @@ -69,6 +70,48 @@ class Plugin { virtual ~Plugin() = default; }; +// A singleton to own PluginRegistrars. This is intended for use in plugins, +// where there is no higher-level object to own a PluginRegistrar that can +// own plugin instances and ensure that they live as long as the engine they +// are registered with. +class PluginRegistrarManager { + public: + static PluginRegistrarManager* GetInstance(); + + // Prevent copying. + PluginRegistrarManager(PluginRegistrarManager const&) = delete; + PluginRegistrarManager& operator=(PluginRegistrarManager const&) = delete; + + // Returns a plugin registrar wrapper of type T, which must be a kind of + // PluginRegistrar, creating it if necessary. The returned registrar will + // live as long as the underlying FlutterDesktopPluginRegistrarRef, so + // can be used to own plugin instances. + // + // Calling this multiple times for the same registrar_ref with different + // template types results in undefined behavior. + template + T* GetRegistrar(FlutterDesktopPluginRegistrarRef registrar_ref) { + auto insert_result = + registrars_.emplace(registrar_ref, std::make_unique(registrar_ref)); + auto& registrar_pair = *(insert_result.first); + FlutterDesktopRegistrarSetDestructionHandler(registrar_pair.first, + OnRegistrarDestroyed); + return static_cast(registrar_pair.second.get()); + } + + private: + PluginRegistrarManager(); + + using WrapperMap = std::map>; + + static void OnRegistrarDestroyed(FlutterDesktopPluginRegistrarRef registrar); + + WrapperMap* registrars() { return ®istrars_; } + + WrapperMap registrars_; +}; + } // namespace flutter #endif // FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_PLUGIN_REGISTRAR_H_ diff --git a/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc b/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc index 7ca7c3de7..92584a93e 100644 --- a/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc +++ b/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc @@ -138,7 +138,7 @@ void BinaryMessengerImpl::SetMessageHandler(const std::string& channel, ForwardToHandler, message_handler); } -// PluginRegistrar: +// ===== PluginRegistrar ===== PluginRegistrar::PluginRegistrar(FlutterDesktopPluginRegistrarRef registrar) : registrar_(registrar) { @@ -157,4 +157,20 @@ void PluginRegistrar::EnableInputBlockingForChannel( FlutterDesktopRegistrarEnableInputBlocking(registrar_, channel.c_str()); } +// ===== PluginRegistrarManager ===== + +// static +PluginRegistrarManager* PluginRegistrarManager::GetInstance() { + static PluginRegistrarManager* instance = new PluginRegistrarManager(); + return instance; +} + +PluginRegistrarManager::PluginRegistrarManager() = default; + +// static +void PluginRegistrarManager::OnRegistrarDestroyed( + FlutterDesktopPluginRegistrarRef registrar) { + GetInstance()->registrars()->erase(registrar); +} + } // namespace flutter diff --git a/shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc b/shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc index 5f1a5b442..eee4758c2 100644 --- a/shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc +++ b/shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc @@ -33,28 +33,36 @@ class TestApi : public testing::StubFlutterApi { return message_engine_result; } - // Called for FlutterDesktopMessengerSetCallback. void MessengerSetCallback(const char* channel, FlutterDesktopMessageCallback callback, void* user_data) override { - last_callback_set_ = callback; + last_message_callback_set_ = callback; + } + + void RegistrarSetDestructionHandler( + FlutterDesktopOnRegistrarDestroyed callback) override { + last_destruction_callback_set_ = callback; } const uint8_t* last_data_sent() { return last_data_sent_; } - FlutterDesktopMessageCallback last_callback_set() { - return last_callback_set_; + FlutterDesktopMessageCallback last_message_callback_set() { + return last_message_callback_set_; + } + FlutterDesktopOnRegistrarDestroyed last_destruction_callback_set() { + return last_destruction_callback_set_; } private: const uint8_t* last_data_sent_ = nullptr; - FlutterDesktopMessageCallback last_callback_set_ = nullptr; + FlutterDesktopMessageCallback last_message_callback_set_ = nullptr; + FlutterDesktopOnRegistrarDestroyed last_destruction_callback_set_ = nullptr; }; } // namespace // Tests that the registrar returns a messenger that passes Send through to the // C API. -TEST(MethodCallTest, MessengerSend) { +TEST(PluginRegistrarTest, MessengerSend) { testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique()); auto test_api = static_cast(scoped_api_stub.stub()); @@ -70,7 +78,7 @@ TEST(MethodCallTest, MessengerSend) { // Tests that the registrar returns a messenger that passes callback // registration and unregistration through to the C API. -TEST(MethodCallTest, MessengerSetMessageHandler) { +TEST(PluginRegistrarTest, MessengerSetMessageHandler) { testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique()); auto test_api = static_cast(scoped_api_stub.stub()); @@ -85,11 +93,60 @@ TEST(MethodCallTest, MessengerSetMessageHandler) { const size_t message_size, BinaryReply reply) {}; messenger->SetMessageHandler(channel_name, std::move(binary_handler)); - EXPECT_NE(test_api->last_callback_set(), nullptr); + EXPECT_NE(test_api->last_message_callback_set(), nullptr); // Unregister. messenger->SetMessageHandler(channel_name, nullptr); - EXPECT_EQ(test_api->last_callback_set(), nullptr); + EXPECT_EQ(test_api->last_message_callback_set(), nullptr); +} + +// Tests that the registrar manager returns the same instance when getting +// the wrapper for the same reference. +TEST(PluginRegistrarTest, ManagerSameInstance) { + testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique()); + + auto dummy_registrar_handle = + reinterpret_cast(1); + + PluginRegistrarManager* manager = PluginRegistrarManager::GetInstance(); + EXPECT_EQ(manager->GetRegistrar(dummy_registrar_handle), + manager->GetRegistrar(dummy_registrar_handle)); +} + +// Tests that the registrar manager returns different objects for different +// references. +TEST(PluginRegistrarTest, ManagerDifferentInstances) { + testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique()); + + auto dummy_registrar_handle_a = + reinterpret_cast(1); + auto dummy_registrar_handle_b = + reinterpret_cast(2); + + PluginRegistrarManager* manager = PluginRegistrarManager::GetInstance(); + EXPECT_NE(manager->GetRegistrar(dummy_registrar_handle_a), + manager->GetRegistrar(dummy_registrar_handle_b)); +} + +// Tests that the registrar manager deletes wrappers when the underlying +// reference is destroyed. +TEST(PluginRegistrarTest, ManagerRemovesOnDestruction) { + testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique()); + auto test_api = static_cast(scoped_api_stub.stub()); + + auto dummy_registrar_handle = + reinterpret_cast(1); + PluginRegistrarManager* manager = PluginRegistrarManager::GetInstance(); + auto* first_wrapper = + manager->GetRegistrar(dummy_registrar_handle); + + // Simulate destruction of the reference. + EXPECT_NE(test_api->last_destruction_callback_set(), nullptr); + test_api->last_destruction_callback_set()(dummy_registrar_handle); + + // Requesting the wrapper should now create a new object. + EXPECT_NE(manager->GetRegistrar(dummy_registrar_handle), + first_wrapper); } } // namespace flutter diff --git a/shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.cc b/shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.cc index 4bd6ddce0..580df4864 100644 --- a/shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.cc +++ b/shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.cc @@ -44,6 +44,14 @@ FlutterDesktopMessengerRef FlutterDesktopRegistrarGetMessenger( return reinterpret_cast(1); } +void FlutterDesktopRegistrarSetDestructionHandler( + FlutterDesktopPluginRegistrarRef registrar, + FlutterDesktopOnRegistrarDestroyed callback) { + if (s_stub_implementation) { + s_stub_implementation->RegistrarSetDestructionHandler(callback); + } +} + void FlutterDesktopRegistrarEnableInputBlocking( FlutterDesktopPluginRegistrarRef registrar, const char* channel) { diff --git a/shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.h b/shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.h index c5f9dbbb2..284d15e97 100644 --- a/shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.h +++ b/shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.h @@ -34,6 +34,10 @@ class StubFlutterApi { virtual ~StubFlutterApi() {} + // Called for FlutterDesktopRegistrarSetDestructionHandler. + virtual void RegistrarSetDestructionHandler( + FlutterDesktopOnRegistrarDestroyed callback) {} + // Called for FlutterDesktopRegistrarEnableInputBlocking. virtual void RegistrarEnableInputBlocking(const char* channel) {} diff --git a/shell/platform/common/cpp/public/flutter_plugin_registrar.h b/shell/platform/common/cpp/public/flutter_plugin_registrar.h index 1caa3cee1..95f0abf13 100644 --- a/shell/platform/common/cpp/public/flutter_plugin_registrar.h +++ b/shell/platform/common/cpp/public/flutter_plugin_registrar.h @@ -18,10 +18,19 @@ extern "C" { // Opaque reference to a plugin registrar. typedef struct FlutterDesktopPluginRegistrar* FlutterDesktopPluginRegistrarRef; +// Function pointer type for registrar destruction callback. +typedef void (*FlutterDesktopOnRegistrarDestroyed)( + FlutterDesktopPluginRegistrarRef); + // Returns the engine messenger associated with this registrar. FLUTTER_EXPORT FlutterDesktopMessengerRef FlutterDesktopRegistrarGetMessenger(FlutterDesktopPluginRegistrarRef registrar); +// Registers a callback to be called when the plugin registrar is destroyed. +FLUTTER_EXPORT void FlutterDesktopRegistrarSetDestructionHandler( + FlutterDesktopPluginRegistrarRef registrar, + FlutterDesktopOnRegistrarDestroyed callback); + // Enables input blocking on the given channel. // // If set, then the Flutter window will disable input callbacks diff --git a/shell/platform/glfw/flutter_glfw.cc b/shell/platform/glfw/flutter_glfw.cc index fa359076c..74a5be634 100644 --- a/shell/platform/glfw/flutter_glfw.cc +++ b/shell/platform/glfw/flutter_glfw.cc @@ -118,6 +118,9 @@ struct FlutterDesktopPluginRegistrar { // The handle for the window associated with this registrar. FlutterDesktopWindow* window; + + // Callback to be called on registrar destruction. + FlutterDesktopOnRegistrarDestroyed destruction_handler; }; // State associated with the messenger used to communicate with the engine. @@ -682,6 +685,11 @@ FlutterDesktopWindowControllerRef FlutterDesktopCreateWindow( } void FlutterDesktopDestroyWindow(FlutterDesktopWindowControllerRef controller) { + FlutterDesktopPluginRegistrarRef registrar = + controller->plugin_registrar.get(); + if (registrar->destruction_handler) { + registrar->destruction_handler(registrar); + } FlutterEngineShutdown(controller->engine); delete controller; } @@ -832,6 +840,12 @@ FlutterDesktopMessengerRef FlutterDesktopRegistrarGetMessenger( return registrar->messenger.get(); } +void FlutterDesktopRegistrarSetDestructionHandler( + FlutterDesktopPluginRegistrarRef registrar, + FlutterDesktopOnRegistrarDestroyed callback) { + registrar->destruction_handler = callback; +} + FlutterDesktopWindowRef FlutterDesktopRegistrarGetWindow( FlutterDesktopPluginRegistrarRef registrar) { return registrar->window; diff --git a/shell/platform/windows/flutter_windows.cc b/shell/platform/windows/flutter_windows.cc index 25da4374b..c53b055ff 100644 --- a/shell/platform/windows/flutter_windows.cc +++ b/shell/platform/windows/flutter_windows.cc @@ -248,6 +248,12 @@ FlutterDesktopMessengerRef FlutterDesktopRegistrarGetMessenger( return registrar->messenger.get(); } +void FlutterDesktopRegistrarSetDestructionHandler( + FlutterDesktopPluginRegistrarRef registrar, + FlutterDesktopOnRegistrarDestroyed callback) { + registrar->destruction_handler = callback; +} + FlutterDesktopViewRef FlutterDesktopRegistrarGetView( FlutterDesktopPluginRegistrarRef registrar) { return registrar->window; diff --git a/shell/platform/windows/win32_flutter_window.cc b/shell/platform/windows/win32_flutter_window.cc index c0c786799..cf6961883 100644 --- a/shell/platform/windows/win32_flutter_window.cc +++ b/shell/platform/windows/win32_flutter_window.cc @@ -15,6 +15,9 @@ Win32FlutterWindow::Win32FlutterWindow(int width, int height) { Win32FlutterWindow::~Win32FlutterWindow() { DestroyRenderSurface(); + if (plugin_registrar_ && plugin_registrar_->destruction_handler) { + plugin_registrar_->destruction_handler(plugin_registrar_.get()); + } } FlutterDesktopViewControllerRef Win32FlutterWindow::CreateWin32FlutterWindow( diff --git a/shell/platform/windows/window_state.h b/shell/platform/windows/window_state.h index 086f6aebd..6f0451d7e 100644 --- a/shell/platform/windows/window_state.h +++ b/shell/platform/windows/window_state.h @@ -55,6 +55,9 @@ struct FlutterDesktopPluginRegistrar { // The handle for the window associated with this registrar. FlutterDesktopView* window; + + // Callback to be called on registrar destruction. + FlutterDesktopOnRegistrarDestroyed destruction_handler; }; // State associated with the messenger used to communicate with the engine. -- GitLab