From 06d28e34dde98f7001c31446a7927782ca74ff58 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 14 Oct 2020 10:36:46 -0700 Subject: [PATCH] Fix destruction order in C++ plugin registrar 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. --- .../include/flutter/plugin_registrar.h | 5 ++ .../cpp/client_wrapper/plugin_registrar.cc | 12 +++- .../plugin_registrar_unittests.cc | 33 ++++++++++ shell/platform/glfw/client_wrapper/BUILD.gn | 1 + .../include/flutter/plugin_registrar_glfw.h | 7 ++- .../plugin_registrar_glfw_unittests.cc | 60 +++++++++++++++++++ .../testing/stub_flutter_glfw_api.cc | 6 ++ .../flutter/plugin_registrar_windows.h | 7 ++- .../plugin_registrar_windows_unittests.cc | 34 +++++++++++ 9 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 shell/platform/glfw/client_wrapper/plugin_registrar_glfw_unittests.cc 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());