WeakPersistentHandle migration

dcharkes · dcharkes · commit 0638046374a6 · 2020-10-02T10:58:39.000+02:00
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': 'ddbeaabe8b3dc6299d9c963d414f820198edf8cd',
+  'dart_revision': 'ffdebe55c9c112d60bd794aabe102f90c305970f',
 
   # WARNING: DO NOT EDIT MANUALLY
   # The lines between blank lines above and below are generated by a script. See create_updated_flutter_deps.py
@@ -46,7 +46,7 @@ vars = {
   'dart_http_retry_tag': '0.1.1',
   'dart_http_throttle_tag': '1.0.2',
   'dart_intl_tag': '0.16.1',
-  'dart_linter_tag': '0.1.119',
+  'dart_linter_tag': '0.1.120',
   'dart_oauth2_tag': '1.6.0',
   'dart_protobuf_rev': '3746c8fd3f2b0147623a8e3db89c3ff4330de760',
   'dart_pub_rev': '04e237f78b2302d7f20d0b362554425e8deb8add',
@@ -155,7 +155,7 @@ deps = {
    'https://boringssl.googlesource.com/boringssl.git' + '@' + Var('dart_boringssl_rev'),
 
   'src/third_party/dart':
-   Var('dart_git') + '/sdk.git' + '@' + Var('dart_revision'),
+   'https://github.com/dart-lang/sdk.git' + '@' + Var('dart_revision'),
 
   # WARNING: Unused Dart dependencies in the list below till "WARNING:" marker are removed automatically - see create_updated_flutter_deps.py.
 
diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter
@@ -1432,6 +1432,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
@@ -223,12 +223,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
@@ -875,6 +875,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
@@ -1910,8 +1910,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
@@ -91,6 +91,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/third_party/tonic/BUILD.gn b/third_party/tonic/BUILD.gn
@@ -29,6 +29,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,65 @@
+// 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) {
+    return;
+  }
+
+  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/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
`@@ -875,6 +875,11 @@ void DartIsolate::AddIsolateShutdownCallback(const fml::closure& closure) {`
`875`	`875`	`}`
`876`	`876`
`877`	`877`	`void DartIsolate::OnShutdownCallback() {`
	`878`	`+ tonic::DartState* state = tonic::DartState::Current();`
	`879`	`+ if (state != nullptr) {`
	`880`	`+ state->SetIsShuttingDown();`
	`881`	`+ }`
	`882`	`+`
`878`	`883`	`{`
`879`	`884`	`tonic::DartApiScope api_scope;`
`880`	`885`	`Dart_Handle sticky_error = Dart_GetStickyError();`
Original file line number	Diff line number	Diff line change
`@@ -91,6 +91,10 @@ void IsolateShutdownCallback(void* isolate_group_data, void* isolate_data) {`
`91`	`91`	`tonic::DartMicrotaskQueue::GetForCurrentThread()->Destroy();`
`92`	`92`	`async_loop_quit(loop);`
`93`	`93`	`}`
	`94`	`+`
	`95`	`+ auto state =`
	`96`	`+ static_cast<std::shared_ptr<tonic::DartState>*>(isolate_group_data);`
	`97`	`+ state->get()->SetIsShuttingDown();`
`94`	`98`	`}`
`95`	`99`
`96`	`100`	`void IsolateGroupCleanupCallback(void* isolate_group_data) {`