From f8e08cc448e962101bb091af6526dfd026206910 Mon Sep 17 00:00:00 2001 From: asiva Date: Fri, 11 Nov 2022 15:58:46 -0800 Subject: [PATCH 1/5] Add calls to Dart_NotifyDestroyed when the flutter view is destroyed. --- runtime/runtime_controller.cc | 13 +++++++++++++ runtime/runtime_controller.h | 11 +++++++++++ shell/common/engine.cc | 5 +++++ shell/common/engine.h | 7 +++++++ shell/common/shell.cc | 6 ++++++ 5 files changed, 42 insertions(+) diff --git a/runtime/runtime_controller.cc b/runtime/runtime_controller.cc index 553c8f5c5e092..aaf279610562c 100644 --- a/runtime/runtime_controller.cc +++ b/runtime/runtime_controller.cc @@ -235,6 +235,19 @@ bool RuntimeController::NotifyIdle(fml::TimePoint deadline) { return true; } +bool RuntimeController::NotifyDestroyed() { + std::shared_ptr root_isolate = root_isolate_.lock(); + if (!root_isolate) { + return false; + } + + tonic::DartState::Scope scope(root_isolate); + + Dart_NotifyDestroyed(); + + return true; +} + bool RuntimeController::DispatchPlatformMessage( std::unique_ptr message) { if (auto* platform_configuration = GetPlatformConfigurationIfAvailable()) { diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h index 20b0d17c4c649..f6c50fbdd8c80 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -360,6 +360,17 @@ class RuntimeController : public PlatformConfigurationClient { /// virtual bool NotifyIdle(fml::TimePoint deadline); + //---------------------------------------------------------------------------- + /// @brief Notify the Dart VM that the attached flutter view has been + /// destroyed. This gives the Dart VM to perform some cleanup + /// activities e.g: perform garbage collection to free up any + /// unused memory. + /// + /// NotifyDestroyed is advisory. The VM may or may not perform any clean up + /// activities. + /// + virtual bool NotifyDestroyed(); + //---------------------------------------------------------------------------- /// @brief Returns if the root isolate is running. The isolate must be /// transitioned to the running phase manually. The isolate can diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 8f19bdc9e0402..85db9043eeff7 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -266,6 +266,11 @@ void Engine::NotifyIdle(fml::TimePoint deadline) { runtime_controller_->NotifyIdle(deadline); } +void Engine::NotifyDestroyed() { + TRACE_EVENT0("flutter", "Engine::NotifyDestroyed"); + runtime_controller_->NotifyDestroyed(); +} + std::optional Engine::GetUIIsolateReturnCode() { return runtime_controller_->GetRootIsolateReturnCode(); } diff --git a/shell/common/engine.h b/shell/common/engine.h index 62f09fd79fbde..82e041e6ff2d1 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -554,6 +554,13 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { /// void NotifyIdle(fml::TimePoint deadline); + //---------------------------------------------------------------------------- + /// @brief Notifies the engine that the attached flutter view has been + /// destroyed. + /// This enables the engine to notify the Dart VM so it can do + /// some cleanp activities. + void NotifyDestroyed(); + //---------------------------------------------------------------------------- /// @brief Dart code cannot fully measure the time it takes for a /// specific frame to be rendered. This is because Dart code only diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 20ca64b8578e9..e42927d5a544b 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -899,6 +899,12 @@ void Shell::OnPlatformViewDestroyed() { // Overall, the longer term plan is to remove this implementation once // https://github.com/flutter/flutter/issues/96679 is fixed. rasterizer_->TeardownExternalViewEmbedder(); + + auto ui_task = [engine = engine_->GetWeakPtr()] { + if (engine) { + engine->NotifyDestroyed(); + } + }; } // |PlatformView::Delegate| From 2f6b39d0dbec3ed08cca015912825aa41d186cf1 Mon Sep 17 00:00:00 2001 From: asiva Date: Fri, 11 Nov 2022 17:43:52 -0800 Subject: [PATCH 2/5] Add unit test case. --- shell/common/fixtures/shell_test.dart | 7 ++++++ shell/common/shell_unittests.cc | 35 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index 9a7c02179fd6b..5545b6ea8f7b4 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -212,6 +212,11 @@ void performanceModeImpactsNotifyIdle() { PlatformDispatcher.instance.requestDartPerformanceMode(DartPerformanceMode.balanced); } +@pragma('vm:entry-point') +void callNotifyDestroyed() { + notifyDestroyedBool(); +} + @pragma('vm:external-name', 'NotifyMessage') external void notifyMessage(String string); @@ -424,6 +429,8 @@ Future runCallback(IsolateParam param) async { @pragma('vm:entry-point') @pragma('vm:external-name', 'NotifyNativeBool') external void notifyNativeBool(bool value); +@pragma('vm:external-name', 'NotifyDestroyedBool') +external void notifyDestroyedBool(); @pragma('vm:entry-point') Future testPluginUtilitiesCallbackHandle() async { diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 91f2a24864902..42c06e1e2f6b4 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -3932,6 +3932,41 @@ TEST_F(ShellTest, NotifyIdleNotCalledInLatencyMode) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } +TEST_F(ShellTest, NotifyDestroyed) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform | ThreadHost::UI | + ThreadHost::IO | ThreadHost::RASTER); + auto platform_task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", thread_host.platform_thread->GetTaskRunner(), + thread_host.raster_thread->GetTaskRunner(), + thread_host.ui_thread->GetTaskRunner(), + thread_host.io_thread->GetTaskRunner()); + auto shell = CreateShell(settings, task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + ASSERT_TRUE(ValidateShell(shell.get())); + + fml::CountDownLatch latch(1); + AddNativeCallback( + "NotifyDestroyedBool", CREATE_NATIVE_ENTRY([&](auto args) { + auto runtime_controller = const_cast( + shell->GetEngine()->GetRuntimeController()); + bool success = runtime_controller->NotifyDestroyed(); + EXPECT_TRUE(success); + latch.CountDown(); + })); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("callNotifyDestroyed"); + RunEngine(shell.get(), std::move(configuration)); + + latch.Wait(); + + DestroyShell(std::move(shell), task_runners); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + } // namespace testing } // namespace flutter From 5ed8eb327045b9f3317421a5dc8d67dfccacca80 Mon Sep 17 00:00:00 2001 From: asiva Date: Fri, 11 Nov 2022 17:50:05 -0800 Subject: [PATCH 3/5] Format. --- shell/common/shell_unittests.cc | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 42c06e1e2f6b4..904f57345d4b0 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -3948,14 +3948,13 @@ TEST_F(ShellTest, NotifyDestroyed) { ASSERT_TRUE(ValidateShell(shell.get())); fml::CountDownLatch latch(1); - AddNativeCallback( - "NotifyDestroyedBool", CREATE_NATIVE_ENTRY([&](auto args) { - auto runtime_controller = const_cast( - shell->GetEngine()->GetRuntimeController()); - bool success = runtime_controller->NotifyDestroyed(); - EXPECT_TRUE(success); - latch.CountDown(); - })); + AddNativeCallback("NotifyDestroyedBool", CREATE_NATIVE_ENTRY([&](auto args) { + auto runtime_controller = const_cast( + shell->GetEngine()->GetRuntimeController()); + bool success = runtime_controller->NotifyDestroyed(); + EXPECT_TRUE(success); + latch.CountDown(); + })); auto configuration = RunConfiguration::InferFromSettings(settings); configuration.SetEntrypoint("callNotifyDestroyed"); From 67e6bdef3dcdaa463e81d91cf0c5ddeed1f06fab Mon Sep 17 00:00:00 2001 From: asiva Date: Tue, 15 Nov 2022 10:42:08 -0800 Subject: [PATCH 4/5] Ensure the destroy task runs. --- shell/common/shell.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index e42927d5a544b..e4ee2ed7a311a 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -900,11 +900,11 @@ void Shell::OnPlatformViewDestroyed() { // https://github.com/flutter/flutter/issues/96679 is fixed. rasterizer_->TeardownExternalViewEmbedder(); - auto ui_task = [engine = engine_->GetWeakPtr()] { + task_runners_.GetUITaskRunner()->PostTask([engine = engine_->GetWeakPtr()]() { if (engine) { engine->NotifyDestroyed(); } - }; + }); } // |PlatformView::Delegate| From e836a5c4db33c85f93b14a1be9a0824a0b3686ea Mon Sep 17 00:00:00 2001 From: asiva Date: Wed, 16 Nov 2022 17:41:53 -0800 Subject: [PATCH 5/5] Address code review comments. --- shell/common/fixtures/shell_test.dart | 6 +++--- shell/common/shell.cc | 14 ++++++++------ shell/common/shell_unittests.cc | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index 5545b6ea8f7b4..605e3d951d230 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -214,7 +214,7 @@ void performanceModeImpactsNotifyIdle() { @pragma('vm:entry-point') void callNotifyDestroyed() { - notifyDestroyedBool(); + notifyDestroyed(); } @pragma('vm:external-name', 'NotifyMessage') @@ -429,8 +429,8 @@ Future runCallback(IsolateParam param) async { @pragma('vm:entry-point') @pragma('vm:external-name', 'NotifyNativeBool') external void notifyNativeBool(bool value); -@pragma('vm:external-name', 'NotifyDestroyedBool') -external void notifyDestroyedBool(); +@pragma('vm:external-name', 'NotifyDestroyed') +external void notifyDestroyed(); @pragma('vm:entry-point') Future testPluginUtilitiesCallbackHandle() async { diff --git a/shell/common/shell.cc b/shell/common/shell.cc index e4ee2ed7a311a..a8bad9ea960b4 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -851,6 +851,14 @@ void Shell::OnPlatformViewDestroyed() { // This incorrect assumption can lead to deadlock. rasterizer_->DisableThreadMergerIfNeeded(); + // Notify the Dart VM that the PlatformView has been destroyed and some + // cleanup activity can be done (e.g: garbage collect the Dart heap). + task_runners_.GetUITaskRunner()->PostTask([engine = engine_->GetWeakPtr()]() { + if (engine) { + engine->NotifyDestroyed(); + } + }); + // Note: // This is a synchronous operation because certain platforms depend on // setup/suspension of all activities that may be interacting with the GPU in @@ -899,12 +907,6 @@ void Shell::OnPlatformViewDestroyed() { // Overall, the longer term plan is to remove this implementation once // https://github.com/flutter/flutter/issues/96679 is fixed. rasterizer_->TeardownExternalViewEmbedder(); - - task_runners_.GetUITaskRunner()->PostTask([engine = engine_->GetWeakPtr()]() { - if (engine) { - engine->NotifyDestroyed(); - } - }); } // |PlatformView::Delegate| diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 904f57345d4b0..c40ca2af356c3 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -3948,7 +3948,7 @@ TEST_F(ShellTest, NotifyDestroyed) { ASSERT_TRUE(ValidateShell(shell.get())); fml::CountDownLatch latch(1); - AddNativeCallback("NotifyDestroyedBool", CREATE_NATIVE_ENTRY([&](auto args) { + AddNativeCallback("NotifyDestroyed", CREATE_NATIVE_ENTRY([&](auto args) { auto runtime_controller = const_cast( shell->GetEngine()->GetRuntimeController()); bool success = runtime_controller->NotifyDestroyed();