Fix destruction order in C++ plugin registrar (flutter#21840)

The C++ wrapper's plugin registrar can own plugins to provided lifetime
management. However, plugins expect the registrar to be valid for the
life of the object, including during destruction, so any owned plugins
must be explicitly cleared before any registrar-specific destruction
happens.

shell/platform/common/cpp/client_wrapper/include/flutter/plugin_registrar.h

@@ -51,6 +51,11 @@ class PluginRegistrar {
  protected:
   FlutterDesktopPluginRegistrarRef registrar() { return registrar_; }
 
+  // Destroys all owned plugins. Subclasses should call this at the beginning of
+  // their destructors to prevent the possibility of an owned plugin trying to
+  // access destroyed state during its own destruction.
+  void ClearPlugins();
+
  private:
   // Handle for interacting with the C API's registrar.
   FlutterDesktopPluginRegistrarRef registrar_;

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

@@ -21,12 +21,22 @@ PluginRegistrar::PluginRegistrar(FlutterDesktopPluginRegistrarRef registrar)
   messenger_ = std::make_unique<BinaryMessengerImpl>(core_messenger);
 }
 
-PluginRegistrar::~PluginRegistrar() {}
+PluginRegistrar::~PluginRegistrar() {
+  // This must always be the first call.
+  ClearPlugins();
+
+  // Explicitly cleared to facilitate testing of destruction order.
+  messenger_.reset();
+}
 
 void PluginRegistrar::AddPlugin(std::unique_ptr<Plugin> plugin) {
   plugins_.insert(std::move(plugin));
 }
 
+void PluginRegistrar::ClearPlugins() {
+  plugins_.clear();
+}
+
 // ===== PluginRegistrarManager =====
 
 // static

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

@@ -80,8 +80,41 @@ class TestPluginRegistrar : public PluginRegistrar {
   std::function<void()> destruction_callback_;
 };
 
+// A test plugin that tries to access registrar state during destruction and
+// reports it out via a flag provided at construction.
+class TestPlugin : public Plugin {
+ public:
+  // registrar_valid_at_destruction will be set at destruction to indicate
+  // whether or not |registrar->messenger()| was non-null.
+  TestPlugin(PluginRegistrar* registrar, bool* registrar_valid_at_destruction)
+      : registrar_(registrar),
+        registrar_valid_at_destruction_(registrar_valid_at_destruction) {}
+  virtual ~TestPlugin() {
+    *registrar_valid_at_destruction_ = registrar_->messenger() != nullptr;
+  }
+
+ private:
+  PluginRegistrar* registrar_;
+  bool* registrar_valid_at_destruction_;
+};
+
 }  // namespace
 
+// Tests that the registrar runs plugin destructors before its own teardown.
+TEST(PluginRegistrarTest, PluginDestroyedBeforeRegistrar) {
+  auto dummy_registrar_handle =
+      reinterpret_cast<FlutterDesktopPluginRegistrarRef>(1);
+  bool registrar_valid_at_destruction = false;
+  {
+    PluginRegistrar registrar(dummy_registrar_handle);
+
+    auto plugin = std::make_unique<TestPlugin>(&registrar,
+                                               &registrar_valid_at_destruction);
+    registrar.AddPlugin(std::move(plugin));
+  }
+  EXPECT_TRUE(registrar_valid_at_destruction);
+}
+
 // Tests that the registrar returns a messenger that passes Send through to the
 // C API.
 TEST(PluginRegistrarTest, MessengerSend) {

shell/platform/glfw/client_wrapper/BUILD.gn

@@ -76,6 +76,7 @@ executable("client_wrapper_glfw_unittests") {
     "flutter_engine_unittests.cc",
     "flutter_window_controller_unittests.cc",
     "flutter_window_unittests.cc",
+    "plugin_registrar_glfw_unittests.cc",
   ]
 
   defines = [ "FLUTTER_DESKTOP_LIBRARY" ]

shell/platform/glfw/client_wrapper/include/flutter/plugin_registrar_glfw.h

@@ -26,7 +26,12 @@ class PluginRegistrarGlfw : public PluginRegistrar {
         FlutterDesktopPluginRegistrarGetWindow(core_registrar));
   }
 
-  virtual ~PluginRegistrarGlfw() = default;
+  virtual ~PluginRegistrarGlfw() {
+    // Must be the first call.
+    ClearPlugins();
+    // Explicitly cleared to facilitate destruction order testing.
+    window_.reset();
+  }
 
   // Prevent copying.
   PluginRegistrarGlfw(PluginRegistrarGlfw const&) = delete;

shell/platform/glfw/client_wrapper/plugin_registrar_glfw_unittests.cc

@@ -0,0 +1,60 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+#include <string>
+
+#include "flutter/shell/platform/glfw/client_wrapper/include/flutter/plugin_registrar_glfw.h"
+#include "flutter/shell/platform/glfw/client_wrapper/testing/stub_flutter_glfw_api.h"
+#include "gtest/gtest.h"
+
+namespace flutter {
+
+namespace {
+
+// A test plugin that tries to access registrar state during destruction and
+// reports it out via a flag provided at construction.
+class TestPlugin : public Plugin {
+ public:
+  // registrar_valid_at_destruction will be set at destruction to indicate
+  // whether or not |registrar->window()| was non-null.
+  TestPlugin(PluginRegistrarGlfw* registrar,
+             bool* registrar_valid_at_destruction)
+      : registrar_(registrar),
+        registrar_valid_at_destruction_(registrar_valid_at_destruction) {}
+  virtual ~TestPlugin() {
+    *registrar_valid_at_destruction_ = registrar_->window() != nullptr;
+  }
+
+ private:
+  PluginRegistrarGlfw* registrar_;
+  bool* registrar_valid_at_destruction_;
+};
+
+}  // namespace
+
+TEST(PluginRegistrarGlfwTest, GetView) {
+  testing::ScopedStubFlutterGlfwApi scoped_api_stub(
+      std::make_unique<testing::StubFlutterGlfwApi>());
+  PluginRegistrarGlfw registrar(
+      reinterpret_cast<FlutterDesktopPluginRegistrarRef>(1));
+  EXPECT_NE(registrar.window(), nullptr);
+}
+
+// Tests that the registrar runs plugin destructors before its own teardown.
+TEST(PluginRegistrarGlfwTest, PluginDestroyedBeforeRegistrar) {
+  auto dummy_registrar_handle =
+      reinterpret_cast<FlutterDesktopPluginRegistrarRef>(1);
+  bool registrar_valid_at_destruction = false;
+  {
+    PluginRegistrarGlfw registrar(dummy_registrar_handle);
+
+    auto plugin = std::make_unique<TestPlugin>(&registrar,
+                                               &registrar_valid_at_destruction);
+    registrar.AddPlugin(std::move(plugin));
+  }
+  EXPECT_TRUE(registrar_valid_at_destruction);
+}
+
+}  // namespace flutter

shell/platform/glfw/client_wrapper/testing/stub_flutter_glfw_api.cc

@@ -183,6 +183,12 @@ FlutterDesktopPluginRegistrarRef FlutterDesktopGetPluginRegistrar(
   return reinterpret_cast<FlutterDesktopPluginRegistrarRef>(2);
 }
 
+FlutterDesktopWindowRef FlutterDesktopPluginRegistrarGetWindow(
+    FlutterDesktopPluginRegistrarRef registrar) {
+  // The stub ignores this, so just return an arbitrary non-zero value.
+  return reinterpret_cast<FlutterDesktopWindowRef>(3);
+}
+
 void FlutterDesktopPluginRegistrarEnableInputBlocking(
     FlutterDesktopPluginRegistrarRef registrar,
     const char* channel) {

shell/platform/windows/client_wrapper/include/flutter/plugin_registrar_windows.h

@@ -36,7 +36,12 @@ class PluginRegistrarWindows : public PluginRegistrar {
         FlutterDesktopPluginRegistrarGetView(core_registrar));
   }
 
-  virtual ~PluginRegistrarWindows() = default;
+  virtual ~PluginRegistrarWindows() {
+    // Must be the first call.
+    ClearPlugins();
+    // Explicitly cleared to facilitate destruction order testing.
+    view_.reset();
+  }
 
   // Prevent copying.
   PluginRegistrarWindows(PluginRegistrarWindows const&) = delete;

shell/platform/windows/client_wrapper/plugin_registrar_windows_unittests.cc

@@ -43,6 +43,25 @@ class TestWindowsApi : public testing::StubFlutterWindowsApi {
   void* last_registered_user_data_ = nullptr;
 };
 
+// A test plugin that tries to access registrar state during destruction and
+// reports it out via a flag provided at construction.
+class TestPlugin : public Plugin {
+ public:
+  // registrar_valid_at_destruction will be set at destruction to indicate
+  // whether or not |registrar->GetView()| was non-null.
+  TestPlugin(PluginRegistrarWindows* registrar,
+             bool* registrar_valid_at_destruction)
+      : registrar_(registrar),
+        registrar_valid_at_destruction_(registrar_valid_at_destruction) {}
+  virtual ~TestPlugin() {
+    *registrar_valid_at_destruction_ = registrar_->GetView() != nullptr;
+  }
+
+ private:
+  PluginRegistrarWindows* registrar_;
+  bool* registrar_valid_at_destruction_;
+};
+
 }  // namespace
 
 TEST(PluginRegistrarWindowsTest, GetView) {
@@ -54,6 +73,21 @@ TEST(PluginRegistrarWindowsTest, GetView) {
   EXPECT_NE(registrar.GetView(), nullptr);
 }
 
+// Tests that the registrar runs plugin destructors before its own teardown.
+TEST(PluginRegistrarWindowsTest, PluginDestroyedBeforeRegistrar) {
+  auto dummy_registrar_handle =
+      reinterpret_cast<FlutterDesktopPluginRegistrarRef>(1);
+  bool registrar_valid_at_destruction = false;
+  {
+    PluginRegistrarWindows registrar(dummy_registrar_handle);
+
+    auto plugin = std::make_unique<TestPlugin>(&registrar,
+                                               &registrar_valid_at_destruction);
+    registrar.AddPlugin(std::move(plugin));
+  }
+  EXPECT_TRUE(registrar_valid_at_destruction);
+}
+
 TEST(PluginRegistrarWindowsTest, RegisterUnregister) {
   testing::ScopedStubFlutterWindowsApi scoped_api_stub(
       std::make_unique<TestWindowsApi>());