From e7ac85910fca68984690e64adc5e62d43387d9a3 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Tue, 13 Apr 2021 17:42:28 -0700 Subject: [PATCH 1/3] Made sure not to delete handles of dart objects if the isolate has been deleted (#25506) --- runtime/dart_isolate.cc | 10 ++- runtime/dart_isolate.h | 4 + runtime/dart_isolate_unittests.cc | 1 + testing/dart_isolate_runner.cc | 15 +++- testing/dart_isolate_runner.h | 2 + third_party/tonic/dart_persistent_value.cc | 17 ++-- third_party/tonic/tests/BUILD.gn | 1 + .../tests/dart_persistent_handle_unittest.cc | 77 +++++++++++++++++++ .../dart_weak_persistent_handle_unittest.cc | 6 +- 9 files changed, 124 insertions(+), 9 deletions(-) create mode 100644 third_party/tonic/tests/dart_persistent_handle_unittest.cc 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(); From 4fdfe81b31efe09939d10439e3dbfc32fd1e5f6f Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 14 Apr 2021 13:06:51 -0700 Subject: [PATCH 2/3] Roll Dart SDK to 2.13.0-211.6.beta --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index ceac474967400..3c1ab2592c4f0 100644 --- a/DEPS +++ b/DEPS @@ -35,7 +35,7 @@ vars = { # Dart is: https://github.com/dart-lang/sdk/blob/master/DEPS. # You can use //tools/dart/create_updated_flutter_deps.py to produce # updated revision list of existing dependencies. - 'dart_revision': 'dda1a31495e39a77b463753caf1295eaa47bc983', + 'dart_revision': '94162a28126443d4c3f1a7c9267ec1d69d38b330', # WARNING: DO NOT EDIT MANUALLY # The lines between blank lines above and below are generated by a script. See create_updated_flutter_deps.py From a0d31ddf00bd0dd42a08f274b2389632775a03ee Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 14 Apr 2021 13:50:04 -0700 Subject: [PATCH 3/3] update licenses golden --- ci/licenses_golden/licenses_third_party | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/licenses_golden/licenses_third_party b/ci/licenses_golden/licenses_third_party index 6f57b39c3b902..851cc1dd408ae 100644 --- a/ci/licenses_golden/licenses_third_party +++ b/ci/licenses_golden/licenses_third_party @@ -1,4 +1,4 @@ -Signature: 157665685c22d5c88a7863dd84ab95a2 +Signature: 0fce0af83c09a7625b27fad90f7ff9eb UNUSED LICENSES: