diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 14e059ac082c7..837073ca9c567 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -203,6 +203,12 @@ std::weak_ptr DartIsolate::CreateRunningRootIsolate( return isolate; } +void DartIsolate::SpawnIsolateShutdownCallback( + std::shared_ptr* isolate_group_data, + std::shared_ptr* isolate_data) { + DartIsolate::DartIsolateShutdownCallback(isolate_group_data, isolate_data); +} + std::weak_ptr DartIsolate::CreateRootIsolate( const Settings& settings, fml::RefPtr isolate_snapshot, @@ -267,7 +273,9 @@ std::weak_ptr DartIsolate::CreateRootIsolate( return Dart_CreateIsolateInGroup( /*group_member=*/spawning_isolate->isolate(), /*name=*/(*isolate_group_data)->GetAdvisoryScriptEntrypoint().c_str(), - /*shutdown_callback=*/nullptr, + /*shutdown_callback=*/ + reinterpret_cast( + DartIsolate::SpawnIsolateShutdownCallback), /*cleanup_callback=*/nullptr, /*child_isolate_data=*/isolate_data, /*error=*/error); diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index 8d4857b32e9e9..d7d00bd707d42 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -552,6 +552,10 @@ class DartIsolate : public UIDartState { // |Dart_DeferredLoadHandler| static Dart_Handle OnDartLoadLibrary(intptr_t loading_unit_id); + static void SpawnIsolateShutdownCallback( + std::shared_ptr* isolate_group_data, + std::shared_ptr* isolate_data); + FML_DISALLOW_COPY_AND_ASSIGN(DartIsolate); }; diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index bb6ada9e3e98d..12a2f0ba1c872 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -153,6 +153,7 @@ TEST_F(DartIsolateTest, SpawnIsolate) { } ASSERT_TRUE(spawn->Shutdown()); + ASSERT_TRUE(spawn->IsShuttingDown()); ASSERT_TRUE(root_isolate->Shutdown()); } diff --git a/testing/dart_isolate_runner.cc b/testing/dart_isolate_runner.cc index 4ce157ee8ec43..6f690cccf483a 100644 --- a/testing/dart_isolate_runner.cc +++ b/testing/dart_isolate_runner.cc @@ -13,12 +13,25 @@ AutoIsolateShutdown::AutoIsolateShutdown(std::shared_ptr isolate, : isolate_(std::move(isolate)), runner_(std::move(runner)) {} AutoIsolateShutdown::~AutoIsolateShutdown() { + if (!isolate_->IsShuttingDown()) { + Shutdown(); + } + fml::AutoResetWaitableEvent latch; + fml::TaskRunner::RunNowOrPostTask(runner_, + [isolate = std::move(isolate_), &latch]() { + // Delete isolate on thread. + latch.Signal(); + }); + latch.Wait(); +} + +void AutoIsolateShutdown::Shutdown() { if (!IsValid()) { return; } fml::AutoResetWaitableEvent latch; fml::TaskRunner::RunNowOrPostTask( - runner_, [isolate = std::move(isolate_), &latch]() { + runner_, [isolate = isolate_.get(), &latch]() { if (!isolate->Shutdown()) { FML_LOG(ERROR) << "Could not shutdown isolate."; FML_CHECK(false); diff --git a/testing/dart_isolate_runner.h b/testing/dart_isolate_runner.h index ab6029e38b02b..40e34110b9807 100644 --- a/testing/dart_isolate_runner.h +++ b/testing/dart_isolate_runner.h @@ -30,6 +30,8 @@ class AutoIsolateShutdown { [[nodiscard]] bool RunInIsolateScope(std::function closure); + void Shutdown(); + DartIsolate* get() { FML_CHECK(isolate_); return isolate_.get(); diff --git a/third_party/tonic/dart_persistent_value.cc b/third_party/tonic/dart_persistent_value.cc index 3bc46505d31fc..652d2c9fc3a1e 100644 --- a/third_party/tonic/dart_persistent_value.cc +++ b/third_party/tonic/dart_persistent_value.cc @@ -43,12 +43,19 @@ void DartPersistentValue::Clear() { return; } - if (Dart_CurrentIsolateGroup()) { - Dart_DeletePersistentHandle(value_); - } else { - DartIsolateScope scope(dart_state->isolate()); - Dart_DeletePersistentHandle(value_); + /// TODO(80155): Remove the handle even if the isolate is shutting down. This + /// may cause memory to stick around until the isolate group is destroyed. + /// Without this branch, if DartState::IsShuttingDown == true, this code will + /// crash when binding the isolate. + if (!dart_state->IsShuttingDown()) { + if (Dart_CurrentIsolateGroup()) { + Dart_DeletePersistentHandle(value_); + } else { + DartIsolateScope scope(dart_state->isolate()); + Dart_DeletePersistentHandle(value_); + } } + dart_state_.reset(); value_ = nullptr; } diff --git a/third_party/tonic/tests/BUILD.gn b/third_party/tonic/tests/BUILD.gn index 359deb13b7904..110a71fdc5bdd 100644 --- a/third_party/tonic/tests/BUILD.gn +++ b/third_party/tonic/tests/BUILD.gn @@ -15,6 +15,7 @@ executable("tonic_unittests") { public_configs = [ "//flutter:export_dynamic_symbols" ] sources = [ + "dart_persistent_handle_unittest.cc", "dart_state_unittest.cc", "dart_weak_persistent_handle_unittest.cc", ] diff --git a/third_party/tonic/tests/dart_persistent_handle_unittest.cc b/third_party/tonic/tests/dart_persistent_handle_unittest.cc new file mode 100644 index 0000000000000..a0d3d39ea12f7 --- /dev/null +++ b/third_party/tonic/tests/dart_persistent_handle_unittest.cc @@ -0,0 +1,77 @@ +// 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 "flutter/testing/dart_isolate_runner.h" +#include "flutter/testing/fixture_test.h" + +namespace flutter { +namespace testing { + +class DartPersistentHandleTest : public FixtureTest { + public: + DartPersistentHandleTest() + : settings_(CreateSettingsForFixture()), + vm_(DartVMRef::Create(settings_)), + thread_(CreateNewThread()), + task_runners_(GetCurrentTestName(), + thread_, + thread_, + thread_, + thread_) {} + + ~DartPersistentHandleTest() = default; + + [[nodiscard]] bool RunWithEntrypoint(const std::string& entrypoint) { + if (running_isolate_) { + return false; + } + auto isolate = RunDartCodeInIsolate(vm_, settings_, task_runners_, + entrypoint, {}, GetFixturesPath()); + if (!isolate || isolate->get()->GetPhase() != DartIsolate::Phase::Running) { + return false; + } + + running_isolate_ = std::move(isolate); + return true; + } + + protected: + Settings settings_; + DartVMRef vm_; + std::unique_ptr running_isolate_; + fml::RefPtr thread_; + TaskRunners task_runners_; + FML_DISALLOW_COPY_AND_ASSIGN(DartPersistentHandleTest); +}; + +TEST_F(DartPersistentHandleTest, ClearAfterShutdown) { + auto persistent_value = tonic::DartPersistentValue(); + + fml::AutoResetWaitableEvent event; + AddNativeCallback("GiveObjectToNative", + CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + + auto dart_state = tonic::DartState::Current(); + ASSERT_TRUE(dart_state); + ASSERT_TRUE(tonic::DartState::Current()); + persistent_value.Set(dart_state, handle); + + event.Signal(); + })); + + ASSERT_TRUE(RunWithEntrypoint("callGiveObjectToNative")); + event.Wait(); + + running_isolate_->Shutdown(); + + fml::AutoResetWaitableEvent clear; + task_runners_.GetUITaskRunner()->PostTask([&] { + persistent_value.Clear(); + clear.Signal(); + }); + clear.Wait(); +} +} // namespace testing +} // namespace flutter diff --git a/third_party/tonic/tests/dart_weak_persistent_handle_unittest.cc b/third_party/tonic/tests/dart_weak_persistent_handle_unittest.cc index 65d5e116cd230..86f922f17ad73 100644 --- a/third_party/tonic/tests/dart_weak_persistent_handle_unittest.cc +++ b/third_party/tonic/tests/dart_weak_persistent_handle_unittest.cc @@ -8,6 +8,10 @@ namespace flutter { namespace testing { +namespace { +void NopFinalizer(void* isolate_callback_data, void* peer) {} +} // namespace + class DartWeakPersistentHandle : public FixtureTest { public: DartWeakPersistentHandle() @@ -45,8 +49,6 @@ class DartWeakPersistentHandle : public FixtureTest { FML_DISALLOW_COPY_AND_ASSIGN(DartWeakPersistentHandle); }; -void NopFinalizer(void* isolate_callback_data, void* peer) {} - TEST_F(DartWeakPersistentHandle, ClearImmediately) { auto weak_persistent_value = tonic::DartWeakPersistentValue();