WeakPersistentHandle migration

dcharkes · dcharkes · commit 1628bbd0ed96 · 2020-11-03T11:28:45.000+01:00
diff --git a/BUILD.gn b/BUILD.gn
@@ -98,6 +98,7 @@ group("flutter") {
       "//flutter/shell/platform/embedder:embedder_proctable_unittests",
       "//flutter/shell/platform/embedder:embedder_unittests",
       "//flutter/testing:testing_unittests",
+      "//flutter/third_party/tonic/tests:tonic_unittests",
       "//flutter/third_party/txt:txt_unittests",
     ]
 
diff --git a/DEPS b/DEPS
@@ -34,7 +34,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': 'd2577410a5016eadc111919355e19d42dd45d816',
+  'dart_revision': '52783837369de45d3372cb6c6b7cdd63e71cd829',
 
   # WARNING: DO NOT EDIT MANUALLY
   # The lines between blank lines above and below are generated by a script. See create_updated_flutter_deps.py
diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter
@@ -1463,6 +1463,8 @@ FILE: ../../../flutter/third_party/tonic/dart_persistent_value.cc
 FILE: ../../../flutter/third_party/tonic/dart_persistent_value.h
 FILE: ../../../flutter/third_party/tonic/dart_state.cc
 FILE: ../../../flutter/third_party/tonic/dart_state.h
+FILE: ../../../flutter/third_party/tonic/dart_weak_persistent_value.cc
+FILE: ../../../flutter/third_party/tonic/dart_weak_persistent_value.h
 FILE: ../../../flutter/third_party/tonic/dart_wrappable.cc
 FILE: ../../../flutter/third_party/tonic/dart_wrappable.h
 FILE: ../../../flutter/third_party/tonic/dart_wrapper_info.h
diff --git a/lib/ui/painting/image_decoder.cc b/lib/ui/painting/image_decoder.cc
@@ -224,12 +224,13 @@ void ImageDecoder::Decode(fml::RefPtr<ImageDescriptor> descriptor,
   FML_DCHECK(callback);
   FML_DCHECK(runners_.GetUITaskRunner()->RunsTasksOnCurrentThread());
 
-  // Always service the callback on the UI thread.
-  auto result = [callback, ui_runner = runners_.GetUITaskRunner()](
+  // Always service the callback (and cleanup the descriptor) on the UI thread.
+  auto result = [callback, descriptor, ui_runner = runners_.GetUITaskRunner()](
                     SkiaGPUObject<SkImage> image,
                     fml::tracing::TraceFlow flow) {
-    ui_runner->PostTask(fml::MakeCopyable(
-        [callback, image = std::move(image), flow = std::move(flow)]() mutable {
+    ui_runner->PostTask(
+        fml::MakeCopyable([callback, descriptor, image = std::move(image),
+                           flow = std::move(flow)]() mutable {
           // We are going to terminate the trace flow here. Flows cannot
           // terminate without a base trace. Add one explicitly.
           TRACE_EVENT0("flutter", "ImageDecodeCallback");
diff --git a/lib/ui/painting/image_encoding.cc b/lib/ui/painting/image_encoding.cc
@@ -35,9 +35,7 @@ enum ImageByteFormat {
   kPNG,
 };
 
-void FinalizeSkData(void* isolate_callback_data,
-                    Dart_WeakPersistentHandle handle,
-                    void* peer) {
+void FinalizeSkData(void* isolate_callback_data, void* peer) {
   SkData* buffer = reinterpret_cast<SkData*>(peer);
   buffer->unref();
 }
diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc
@@ -981,6 +981,11 @@ void DartIsolate::AddIsolateShutdownCallback(const fml::closure& closure) {
 }
 
 void DartIsolate::OnShutdownCallback() {
+  tonic::DartState* state = tonic::DartState::Current();
+  if (state != nullptr) {
+    state->SetIsShuttingDown();
+  }
+
   {
     tonic::DartApiScope api_scope;
     Dart_Handle sticky_error = Dart_GetStickyError();
diff --git a/shell/platform/embedder/embedder.cc b/shell/platform/embedder/embedder.cc
@@ -1922,8 +1922,7 @@ FlutterEngineResult FlutterEnginePostDartObject(
         dart_object.value.as_external_typed_data.data = buffer;
         dart_object.value.as_external_typed_data.peer = peer;
         dart_object.value.as_external_typed_data.callback =
-            +[](void* unused_isolate_callback_data,
-                Dart_WeakPersistentHandle unused_handle, void* peer) {
+            +[](void* unused_isolate_callback_data, void* peer) {
               auto typed_peer = reinterpret_cast<ExternalTypedDataPeer*>(peer);
               typed_peer->trampoline(typed_peer->user_data);
               delete typed_peer;
diff --git a/shell/platform/fuchsia/dart_runner/dart_runner.cc b/shell/platform/fuchsia/dart_runner/dart_runner.cc
@@ -86,6 +86,10 @@ void IsolateShutdownCallback(void* isolate_group_data, void* isolate_data) {
     tonic::DartMicrotaskQueue::GetForCurrentThread()->Destroy();
     async_loop_quit(loop);
   }
+
+  auto state =
+      static_cast<std::shared_ptr<tonic::DartState>*>(isolate_group_data);
+  state->get()->SetIsShuttingDown();
 }
 
 void IsolateGroupCleanupCallback(void* isolate_group_data) {
diff --git a/testing/run_tests.py b/testing/run_tests.py
@@ -132,6 +132,8 @@ def RunCCTests(build_dir, filter):
 
   RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags)
 
+  RunEngineExecutable(build_dir, 'tonic_unittests', filter, shuffle_flags)
+
   if not IsWindows():
     # https://github.com/flutter/flutter/issues/36295
     RunEngineExecutable(build_dir, 'shell_unittests', filter, shuffle_flags)
diff --git a/third_party/tonic/BUILD.gn b/third_party/tonic/BUILD.gn
@@ -35,6 +35,8 @@ source_set("tonic") {
     "dart_persistent_value.h",
     "dart_state.cc",
     "dart_state.h",
+    "dart_weak_persistent_value.cc",
+    "dart_weak_persistent_value.h",
     "dart_wrappable.cc",
     "dart_wrappable.h",
     "dart_wrapper_info.h",
diff --git a/third_party/tonic/dart_state.cc b/third_party/tonic/dart_state.cc
@@ -27,7 +27,8 @@ DartState::DartState(int dirfd,
       message_handler_(new DartMessageHandler()),
       file_loader_(new FileLoader(dirfd)),
       message_epilogue_(message_epilogue),
-      has_set_return_code_(false) {}
+      has_set_return_code_(false),
+      is_shutting_down_(false) {}
 
 DartState::~DartState() {}
 
diff --git a/third_party/tonic/dart_state.h b/third_party/tonic/dart_state.h
@@ -5,6 +5,7 @@
 #ifndef LIB_TONIC_DART_STATE_H_
 #define LIB_TONIC_DART_STATE_H_
 
+#include <atomic>
 #include <functional>
 #include <memory>
 
@@ -68,6 +69,9 @@ class DartState : public std::enable_shared_from_this<DartState> {
   void SetReturnCodeCallback(std::function<void(uint32_t)> callback);
   bool has_set_return_code() const { return has_set_return_code_; }
 
+  void SetIsShuttingDown() { is_shutting_down_ = true; }
+  bool IsShuttingDown() { return is_shutting_down_; }
+
   virtual void DidSetIsolate();
 
   static Dart_Handle HandleLibraryTag(Dart_LibraryTag tag,
@@ -83,6 +87,7 @@ class DartState : public std::enable_shared_from_this<DartState> {
   std::function<void(Dart_Handle)> message_epilogue_;
   std::function<void(uint32_t)> set_return_code_callback_;
   bool has_set_return_code_;
+  std::atomic<bool> is_shutting_down_;
 
  protected:
   TONIC_DISALLOW_COPY_AND_ASSIGN(DartState);
diff --git a/third_party/tonic/dart_weak_persistent_value.cc b/third_party/tonic/dart_weak_persistent_value.cc
@@ -0,0 +1,70 @@
+// 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 "tonic/dart_weak_persistent_value.h"
+
+#include "tonic/dart_state.h"
+#include "tonic/scopes/dart_isolate_scope.h"
+
+namespace tonic {
+
+DartWeakPersistentValue::DartWeakPersistentValue() : handle_(nullptr) {}
+
+DartWeakPersistentValue::~DartWeakPersistentValue() {
+  Clear();
+}
+
+void DartWeakPersistentValue::Set(DartState* dart_state,
+                                  Dart_Handle object,
+                                  void* peer,
+                                  intptr_t external_allocation_size,
+                                  Dart_HandleFinalizer callback) {
+  TONIC_DCHECK(is_empty());
+  dart_state_ = dart_state->GetWeakPtr();
+  handle_ = Dart_NewWeakPersistentHandle(object, peer, external_allocation_size,
+                                         callback);
+}
+
+void DartWeakPersistentValue::Clear() {
+  if (!handle_) {
+    return;
+  }
+
+  auto dart_state = dart_state_.lock();
+  if (!dart_state) {
+    // The DartVM that the handle used to belong to has been shut down and that
+    // handle has already been deleted.
+    handle_ = nullptr;
+    return;
+  }
+
+  // The DartVM frees the handles during shutdown and calls the finalizers.
+  // Freeing handles during shutdown would fail.
+  if (!dart_state->IsShuttingDown()) {
+    if (Dart_CurrentIsolateGroup()) {
+      Dart_DeleteWeakPersistentHandle(handle_);
+    } else {
+      // If we are not on the mutator thread, this will fail. The caller must
+      // ensure to be on the mutator thread.
+      DartIsolateScope scope(dart_state->isolate());
+      Dart_DeleteWeakPersistentHandle(handle_);
+    }
+  }
+  // If it's shutting down, the handle will be deleted already.
+
+  dart_state_.reset();
+  handle_ = nullptr;
+}
+
+Dart_Handle DartWeakPersistentValue::Get() {
+  auto dart_state = dart_state_.lock();
+  TONIC_DCHECK(dart_state);
+  TONIC_DCHECK(!dart_state->IsShuttingDown());
+  if (!handle_) {
+    return nullptr;
+  }
+  return Dart_HandleFromWeakPersistent(handle_);
+}
+
+}  // namespace tonic
diff --git a/third_party/tonic/dart_weak_persistent_value.h b/third_party/tonic/dart_weak_persistent_value.h
@@ -0,0 +1,48 @@
+// 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.
+
+#ifndef LIB_TONIC_DART_WEAK_PERSISTENT_VALUE_H_
+#define LIB_TONIC_DART_WEAK_PERSISTENT_VALUE_H_
+
+#include <memory>
+
+#include "third_party/dart/runtime/include/dart_api.h"
+#include "tonic/common/macros.h"
+
+namespace tonic {
+class DartState;
+
+// DartWeakPersistentValue is a bookkeeping class to help pair calls to
+// Dart_NewWeakPersistentHandle with Dart_DeleteWeakPersistentHandle even in
+// the case if IsolateGroup shutdown. Consider using this class instead of
+// holding a Dart_PersistentHandle directly so that you don't leak the
+// Dart_WeakPersistentHandle.
+class DartWeakPersistentValue {
+ public:
+  DartWeakPersistentValue();
+  ~DartWeakPersistentValue();
+
+  Dart_WeakPersistentHandle value() const { return handle_; }
+  bool is_empty() const { return handle_ == nullptr; }
+
+  void Set(DartState* dart_state,
+           Dart_Handle object,
+           void* peer,
+           intptr_t external_allocation_size,
+           Dart_HandleFinalizer callback);
+  void Clear();
+  Dart_Handle Get();
+
+  const std::weak_ptr<DartState>& dart_state() const { return dart_state_; }
+
+ private:
+  std::weak_ptr<DartState> dart_state_;
+  Dart_WeakPersistentHandle handle_;
+
+  TONIC_DISALLOW_COPY_AND_ASSIGN(DartWeakPersistentValue);
+};
+
+}  // namespace tonic
+
+#endif  // LIB_TONIC_DART_WEAK_PERSISTENT_VALUE_H_
diff --git a/third_party/tonic/dart_wrappable.cc b/third_party/tonic/dart_wrappable.cc
@@ -12,12 +12,17 @@
 namespace tonic {
 
 DartWrappable::~DartWrappable() {
-  TONIC_CHECK(!dart_wrapper_);
+  // Calls the destructor of dart_wrapper_ to delete WeakPersistentHandle.
 }
 
 // TODO(dnfield): Delete this. https://github.com/flutter/flutter/issues/50997
 Dart_Handle DartWrappable::CreateDartWrapper(DartState* dart_state) {
-  TONIC_DCHECK(!dart_wrapper_);
+  if (!dart_wrapper_.is_empty()) {
+    // Any previously given out wrapper must have been GCed.
+    TONIC_DCHECK(Dart_IsNull(dart_wrapper_.Get()));
+    dart_wrapper_.Clear();
+  }
+
   const DartWrapperInfo& info = GetDartWrapperInfo();
 
   Dart_PersistentHandle type = dart_state->class_library().GetClass(info);
@@ -36,14 +41,19 @@ Dart_Handle DartWrappable::CreateDartWrapper(DartState* dart_state) {
   TONIC_DCHECK(!LogIfError(res));
 
   this->RetainDartWrappableReference();  // Balanced in FinalizeDartWrapper.
-  dart_wrapper_ = Dart_NewWeakPersistentHandle(
-      wrapper, this, GetAllocationSize(), &FinalizeDartWrapper);
+  dart_wrapper_.Set(dart_state, wrapper, this, GetAllocationSize(),
+                    &FinalizeDartWrapper);
 
   return wrapper;
 }
 
 void DartWrappable::AssociateWithDartWrapper(Dart_Handle wrapper) {
-  TONIC_DCHECK(!dart_wrapper_);
+  if (!dart_wrapper_.is_empty()) {
+    // Any previously given out wrapper must have been GCed.
+    TONIC_DCHECK(Dart_IsNull(dart_wrapper_.Get()));
+    dart_wrapper_.Clear();
+  }
+
   TONIC_CHECK(!LogIfError(wrapper));
 
   const DartWrapperInfo& info = GetDartWrapperInfo();
@@ -54,26 +64,25 @@ void DartWrappable::AssociateWithDartWrapper(Dart_Handle wrapper) {
       wrapper, kWrapperInfoIndex, reinterpret_cast<intptr_t>(&info))));
 
   this->RetainDartWrappableReference();  // Balanced in FinalizeDartWrapper.
-  dart_wrapper_ = Dart_NewWeakPersistentHandle(
-      wrapper, this, GetAllocationSize(), &FinalizeDartWrapper);
+
+  DartState* dart_state = DartState::Current();
+  dart_wrapper_.Set(dart_state, wrapper, this, GetAllocationSize(),
+                    &FinalizeDartWrapper);
 }
 
 void DartWrappable::ClearDartWrapper() {
-  TONIC_DCHECK(dart_wrapper_);
-  Dart_Handle wrapper = Dart_HandleFromWeakPersistent(dart_wrapper_);
+  TONIC_DCHECK(!dart_wrapper_.is_empty());
+  Dart_Handle wrapper = dart_wrapper_.Get();
   TONIC_CHECK(!LogIfError(Dart_SetNativeInstanceField(wrapper, kPeerIndex, 0)));
   TONIC_CHECK(
       !LogIfError(Dart_SetNativeInstanceField(wrapper, kWrapperInfoIndex, 0)));
-  Dart_DeleteWeakPersistentHandle(dart_wrapper_);
-  dart_wrapper_ = nullptr;
+  dart_wrapper_.Clear();
   this->ReleaseDartWrappableReference();
 }
 
 void DartWrappable::FinalizeDartWrapper(void* isolate_callback_data,
-                                        Dart_WeakPersistentHandle wrapper,
                                         void* peer) {
   DartWrappable* wrappable = reinterpret_cast<DartWrappable*>(peer);
-  wrappable->dart_wrapper_ = nullptr;
   wrappable->ReleaseDartWrappableReference();  // Balanced in CreateDartWrapper.
 }
 
diff --git a/third_party/tonic/dart_wrappable.h b/third_party/tonic/dart_wrappable.h
diff --git a/third_party/tonic/tests/BUILD.gn b/third_party/tonic/tests/BUILD.gn
diff --git a/third_party/tonic/tests/dart_state_unittest.cc b/third_party/tonic/tests/dart_state_unittest.cc
diff --git a/third_party/tonic/tests/dart_weak_persistent_handle_unittest.cc b/third_party/tonic/tests/dart_weak_persistent_handle_unittest.cc
diff --git a/third_party/tonic/tests/fixtures/tonic_test.dart b/third_party/tonic/tests/fixtures/tonic_test.dart
diff --git a/third_party/tonic/typed_data/dart_byte_data.cc b/third_party/tonic/typed_data/dart_byte_data.cc

Original file line number	Diff line number	Diff line change
`@@ -98,6 +98,7 @@ group("flutter") {`
`98`	`98`	`"//flutter/shell/platform/embedder:embedder_proctable_unittests",`
`99`	`99`	`"//flutter/shell/platform/embedder:embedder_unittests",`
`100`	`100`	`"//flutter/testing:testing_unittests",`
	`101`	`+ "//flutter/third_party/tonic/tests:tonic_unittests",`
`101`	`102`	`"//flutter/third_party/txt:txt_unittests",`
`102`	`103`	`]`
`103`	`104`
Original file line number	Diff line number	Diff line change
`@@ -981,6 +981,11 @@ void DartIsolate::AddIsolateShutdownCallback(const fml::closure& closure) {`
`981`	`981`	`}`
`982`	`982`
`983`	`983`	`void DartIsolate::OnShutdownCallback() {`
	`984`	`+ tonic::DartState* state = tonic::DartState::Current();`
	`985`	`+ if (state != nullptr) {`
	`986`	`+ state->SetIsShuttingDown();`
	`987`	`+ }`
	`988`	`+`
`984`	`989`	`{`
`985`	`990`	`tonic::DartApiScope api_scope;`
`986`	`991`	`Dart_Handle sticky_error = Dart_GetStickyError();`
Original file line number	Diff line number	Diff line change
`@@ -86,6 +86,10 @@ void IsolateShutdownCallback(void* isolate_group_data, void* isolate_data) {`
`86`	`86`	`tonic::DartMicrotaskQueue::GetForCurrentThread()->Destroy();`
`87`	`87`	`async_loop_quit(loop);`
`88`	`88`	`}`
	`89`	`+`
	`90`	`+ auto state =`
	`91`	`+ static_cast<std::shared_ptr<tonic::DartState>*>(isolate_group_data);`
	`92`	`+ state->get()->SetIsShuttingDown();`
`89`	`93`	`}`
`90`	`94`
`91`	`95`	`void IsolateGroupCleanupCallback(void* isolate_group_data) {`