Skip to content

Commit

Permalink
WeakPersistentHandle migration (flutter#19843)
Browse files Browse the repository at this point in the history
and roll Dart to 5278383.
  • Loading branch information
dcharkes authored Nov 3, 2020
1 parent 78a0181 commit ccdb681
Show file tree
Hide file tree
Showing 22 changed files with 488 additions and 42 deletions.
1 change: 1 addition & 0 deletions BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
]

Expand Down
2 changes: 1 addition & 1 deletion DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion ci/licenses_golden/licenses_third_party
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Signature: c55e82fda8f939e4cdbeb9c2c004e7d9
Signature: 51a79de001f82d05f084b671f6216a31

UNUSED LICENSES:

Expand Down
9 changes: 5 additions & 4 deletions lib/ui/painting/image_decoder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
4 changes: 1 addition & 3 deletions lib/ui/painting/image_encoding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
5 changes: 5 additions & 0 deletions runtime/dart_isolate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
3 changes: 1 addition & 2 deletions shell/platform/embedder/embedder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions shell/platform/fuchsia/dart_runner/dart_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 2 additions & 0 deletions testing/run_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions third_party/tonic/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 2 additions & 1 deletion third_party/tonic/dart_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}

Expand Down
5 changes: 5 additions & 0 deletions third_party/tonic/dart_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef LIB_TONIC_DART_STATE_H_
#define LIB_TONIC_DART_STATE_H_

#include <atomic>
#include <functional>
#include <memory>

Expand Down Expand Up @@ -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,
Expand All @@ -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);
Expand Down
70 changes: 70 additions & 0 deletions third_party/tonic/dart_weak_persistent_value.cc
Original file line number Diff line number Diff line change
@@ -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
48 changes: 48 additions & 0 deletions third_party/tonic/dart_weak_persistent_value.h
Original file line number Diff line number Diff line change
@@ -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_
35 changes: 22 additions & 13 deletions third_party/tonic/dart_wrappable.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
Expand All @@ -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.
}

Expand Down
Loading

0 comments on commit ccdb681

Please sign in to comment.