Reland "Improve C++ plugin lifetime handling" (flutter#17570)

Relands flutter#17489 with a fix for the unit test flake.

The previous unit test relied on the new instance not being created at the same memory address, which isn't guaranteed.

Author: stuartmorgan-g

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" ]
 }

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 <map>
 #include <memory>
 #include <set>
 #include <string>
@@ -69,6 +70,53 @@ 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 <class T>
+  T* GetRegistrar(FlutterDesktopPluginRegistrarRef registrar_ref) {
+    auto insert_result =
+        registrars_.emplace(registrar_ref, std::make_unique<T>(registrar_ref));
+    auto& registrar_pair = *(insert_result.first);
+    FlutterDesktopRegistrarSetDestructionHandler(registrar_pair.first,
+                                                 OnRegistrarDestroyed);
+    return static_cast<T*>(registrar_pair.second.get());
+  }
+
+  // Destroys all registrar wrappers created by the manager.
+  //
+  // This is intended primarily for use in tests.
+  void Reset() { registrars_.clear(); }
+
+ private:
+  PluginRegistrarManager();
+
+  using WrapperMap = std::map<FlutterDesktopPluginRegistrarRef,
+                              std::unique_ptr<PluginRegistrar>>;
+
+  static void OnRegistrarDestroyed(FlutterDesktopPluginRegistrarRef registrar);
+
+  WrapperMap* registrars() { return &registrars_; }
+
+  WrapperMap registrars_;
+};
+
 }  // namespace flutter
 
 #endif  // FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_PLUGIN_REGISTRAR_H_

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

shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc

@@ -33,28 +33,56 @@ 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;
+};
+
+// A PluginRegistrar whose destruction can be watched for by tests.
+class TestPluginRegistrar : public PluginRegistrar {
+ public:
+  explicit TestPluginRegistrar(FlutterDesktopPluginRegistrarRef core_registrar)
+      : PluginRegistrar(core_registrar) {}
+
+  virtual ~TestPluginRegistrar() {
+    if (destruction_callback_) {
+      destruction_callback_();
+    }
+  }
+
+  void SetDestructionCallback(std::function<void()> callback) {
+    destruction_callback_ = std::move(callback);
+  }
+
+ private:
+  std::function<void()> destruction_callback_;
 };
 
 }  // 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<TestApi>());
   auto test_api = static_cast<TestApi*>(scoped_api_stub.stub());
 
@@ -70,7 +98,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<TestApi>());
   auto test_api = static_cast<TestApi*>(scoped_api_stub.stub());
 
@@ -85,11 +113,70 @@ 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) {
+  PluginRegistrarManager* manager = PluginRegistrarManager::GetInstance();
+  manager->Reset();
+
+  testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique<TestApi>());
+
+  auto dummy_registrar_handle =
+      reinterpret_cast<FlutterDesktopPluginRegistrarRef>(1);
+
+  EXPECT_EQ(manager->GetRegistrar<PluginRegistrar>(dummy_registrar_handle),
+            manager->GetRegistrar<PluginRegistrar>(dummy_registrar_handle));
+}
+
+// Tests that the registrar manager returns different objects for different
+// references.
+TEST(PluginRegistrarTest, ManagerDifferentInstances) {
+  PluginRegistrarManager* manager = PluginRegistrarManager::GetInstance();
+  manager->Reset();
+
+  testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique<TestApi>());
+
+  auto dummy_registrar_handle_a =
+      reinterpret_cast<FlutterDesktopPluginRegistrarRef>(1);
+  auto dummy_registrar_handle_b =
+      reinterpret_cast<FlutterDesktopPluginRegistrarRef>(2);
+
+  EXPECT_NE(manager->GetRegistrar<PluginRegistrar>(dummy_registrar_handle_a),
+            manager->GetRegistrar<PluginRegistrar>(dummy_registrar_handle_b));
+}
+
+// Tests that the registrar manager deletes wrappers when the underlying
+// reference is destroyed.
+TEST(PluginRegistrarTest, ManagerRemovesOnDestruction) {
+  PluginRegistrarManager* manager = PluginRegistrarManager::GetInstance();
+  manager->Reset();
+
+  testing::ScopedStubFlutterApi scoped_api_stub(std::make_unique<TestApi>());
+  auto test_api = static_cast<TestApi*>(scoped_api_stub.stub());
+
+  auto dummy_registrar_handle =
+      reinterpret_cast<FlutterDesktopPluginRegistrarRef>(1);
+  auto* wrapper =
+      manager->GetRegistrar<TestPluginRegistrar>(dummy_registrar_handle);
+
+  // Simulate destruction of the reference, and ensure that the wrapper
+  // is destroyed.
+  EXPECT_NE(test_api->last_destruction_callback_set(), nullptr);
+  bool destroyed = false;
+  wrapper->SetDestructionCallback([&destroyed]() { destroyed = true; });
+  test_api->last_destruction_callback_set()(dummy_registrar_handle);
+  EXPECT_EQ(destroyed, true);
+
+  // Requesting the wrapper should now create a new object.
+  EXPECT_NE(manager->GetRegistrar<TestPluginRegistrar>(dummy_registrar_handle),
+            nullptr);
 }
 
 }  // namespace flutter

shell/platform/common/cpp/client_wrapper/testing/stub_flutter_api.cc

@@ -44,6 +44,14 @@ FlutterDesktopMessengerRef FlutterDesktopRegistrarGetMessenger(
   return reinterpret_cast<FlutterDesktopMessengerRef>(1);
 }
 
+void FlutterDesktopRegistrarSetDestructionHandler(
+    FlutterDesktopPluginRegistrarRef registrar,
+    FlutterDesktopOnRegistrarDestroyed callback) {
+  if (s_stub_implementation) {
+    s_stub_implementation->RegistrarSetDestructionHandler(callback);
+  }
+}
+
 void FlutterDesktopRegistrarEnableInputBlocking(
     FlutterDesktopPluginRegistrarRef registrar,
     const char* channel) {

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) {}
 

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

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;

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;

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(