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 06a34e0bf9a29..a01b75e8c9136 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 @@ -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_; diff --git a/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc b/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc index b81411fb40e01..b5ab707c80aec 100644 --- a/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc +++ b/shell/platform/common/cpp/client_wrapper/plugin_registrar.cc @@ -21,12 +21,22 @@ PluginRegistrar::PluginRegistrar(FlutterDesktopPluginRegistrarRef registrar) messenger_ = std::make_unique(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) { plugins_.insert(std::move(plugin)); } +void PluginRegistrar::ClearPlugins() { + plugins_.clear(); +} + // ===== PluginRegistrarManager ===== // static 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 739b959a8814b..b3a92aa67d714 100644 --- a/shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc +++ b/shell/platform/common/cpp/client_wrapper/plugin_registrar_unittests.cc @@ -80,8 +80,41 @@ class TestPluginRegistrar : public PluginRegistrar { std::function 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(1); + bool registrar_valid_at_destruction = false; + { + PluginRegistrar registrar(dummy_registrar_handle); + + auto plugin = std::make_unique(®istrar, + ®istrar_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) { diff --git a/shell/platform/glfw/client_wrapper/BUILD.gn b/shell/platform/glfw/client_wrapper/BUILD.gn index 3a4f57263ef6f..c0d6ca643896f 100644 --- a/shell/platform/glfw/client_wrapper/BUILD.gn +++ b/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" ] diff --git a/shell/platform/glfw/client_wrapper/include/flutter/plugin_registrar_glfw.h b/shell/platform/glfw/client_wrapper/include/flutter/plugin_registrar_glfw.h index 1d0a83e20b2f9..2459dde60bf29 100644 --- a/shell/platform/glfw/client_wrapper/include/flutter/plugin_registrar_glfw.h +++ b/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; diff --git a/shell/platform/glfw/client_wrapper/plugin_registrar_glfw_unittests.cc b/shell/platform/glfw/client_wrapper/plugin_registrar_glfw_unittests.cc new file mode 100644 index 0000000000000..ff95921fb35e3 --- /dev/null +++ b/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 +#include + +#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()); + PluginRegistrarGlfw registrar( + reinterpret_cast(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(1); + bool registrar_valid_at_destruction = false; + { + PluginRegistrarGlfw registrar(dummy_registrar_handle); + + auto plugin = std::make_unique(®istrar, + ®istrar_valid_at_destruction); + registrar.AddPlugin(std::move(plugin)); + } + EXPECT_TRUE(registrar_valid_at_destruction); +} + +} // namespace flutter diff --git a/shell/platform/glfw/client_wrapper/testing/stub_flutter_glfw_api.cc b/shell/platform/glfw/client_wrapper/testing/stub_flutter_glfw_api.cc index c12e54fd4dad7..df61b34550715 100644 --- a/shell/platform/glfw/client_wrapper/testing/stub_flutter_glfw_api.cc +++ b/shell/platform/glfw/client_wrapper/testing/stub_flutter_glfw_api.cc @@ -183,6 +183,12 @@ FlutterDesktopPluginRegistrarRef FlutterDesktopGetPluginRegistrar( return reinterpret_cast(2); } +FlutterDesktopWindowRef FlutterDesktopPluginRegistrarGetWindow( + FlutterDesktopPluginRegistrarRef registrar) { + // The stub ignores this, so just return an arbitrary non-zero value. + return reinterpret_cast(3); +} + void FlutterDesktopPluginRegistrarEnableInputBlocking( FlutterDesktopPluginRegistrarRef registrar, const char* channel) { diff --git a/shell/platform/windows/client_wrapper/include/flutter/plugin_registrar_windows.h b/shell/platform/windows/client_wrapper/include/flutter/plugin_registrar_windows.h index 20b00aeb605eb..98093e1b396e7 100644 --- a/shell/platform/windows/client_wrapper/include/flutter/plugin_registrar_windows.h +++ b/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; diff --git a/shell/platform/windows/client_wrapper/plugin_registrar_windows_unittests.cc b/shell/platform/windows/client_wrapper/plugin_registrar_windows_unittests.cc index 63049a5eb82b9..0c9bdb9618907 100644 --- a/shell/platform/windows/client_wrapper/plugin_registrar_windows_unittests.cc +++ b/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(1); + bool registrar_valid_at_destruction = false; + { + PluginRegistrarWindows registrar(dummy_registrar_handle); + + auto plugin = std::make_unique(®istrar, + ®istrar_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());