Made sure not to delete handles of dart objects if the isolate has been deleted (#25506)

runtime/dart_isolate.cc

@@ -203,6 +203,12 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRunningRootIsolate(
   return isolate;
 }
 
+void DartIsolate::SpawnIsolateShutdownCallback(
+    std::shared_ptr<DartIsolateGroupData>* isolate_group_data,
+    std::shared_ptr<DartIsolate>* isolate_data) {
+  DartIsolate::DartIsolateShutdownCallback(isolate_group_data, isolate_data);
+}
+
 std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
     const Settings& settings,
     fml::RefPtr<const DartSnapshot> isolate_snapshot,
@@ -267,7 +273,9 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
       return Dart_CreateIsolateInGroup(
           /*group_member=*/spawning_isolate->isolate(),
           /*name=*/(*isolate_group_data)->GetAdvisoryScriptEntrypoint().c_str(),
-          /*shutdown_callback=*/nullptr,
+          /*shutdown_callback=*/
+          reinterpret_cast<Dart_IsolateShutdownCallback>(
+              DartIsolate::SpawnIsolateShutdownCallback),
           /*cleanup_callback=*/nullptr,
           /*child_isolate_data=*/isolate_data,
           /*error=*/error);

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<DartIsolateGroupData>* isolate_group_data,
+      std::shared_ptr<DartIsolate>* isolate_data);
+
   FML_DISALLOW_COPY_AND_ASSIGN(DartIsolate);
 };
 

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());
 }
 

testing/dart_isolate_runner.cc

@@ -13,12 +13,25 @@ AutoIsolateShutdown::AutoIsolateShutdown(std::shared_ptr<DartIsolate> 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);

testing/dart_isolate_runner.h

@@ -30,6 +30,8 @@ class AutoIsolateShutdown {
 
   [[nodiscard]] bool RunInIsolateScope(std::function<bool(void)> closure);
 
+  void Shutdown();
+
   DartIsolate* get() {
     FML_CHECK(isolate_);
     return isolate_.get();

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;
 }

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",
   ]

third_party/tonic/tests/dart_persistent_handle_unittest.cc

+// 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<AutoIsolateShutdown> running_isolate_;
+  fml::RefPtr<fml::TaskRunner> 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

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();