From 7a6b77999feab73c03a0c65a3371beaeb750cced Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Wed, 18 Apr 2018 13:02:19 -0700 Subject: [PATCH] Remove the weak pointer factory from the service isolate's DartIsolate object (#5041) The WeakPtrFactory must be deleted on the thread where it was created. However, the service isolate is created and destroyed on threads from the Dart thread pool, and the creating thread may not be the same as the destroying thread. --- lib/ui/ui_dart_state.cc | 3 +-- lib/ui/ui_dart_state.h | 1 - runtime/dart_isolate.cc | 52 ++++++++++++++++++++++++++--------------- runtime/dart_isolate.h | 12 +++++++++- 4 files changed, 45 insertions(+), 23 deletions(-) diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index 8ef7ca98f..4f3de1b22 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -28,8 +28,7 @@ UIDartState::UIDartState(TaskRunners task_runners, advisory_script_uri_(std::move(advisory_script_uri)), advisory_script_entrypoint_(std::move(advisory_script_entrypoint)), logger_prefix_(std::move(logger_prefix)), - skia_unref_queue_(std::move(skia_unref_queue)), - weak_factory_(this) { + skia_unref_queue_(std::move(skia_unref_queue)) { AddOrRemoveTaskObserver(true /* add */); } diff --git a/lib/ui/ui_dart_state.h b/lib/ui/ui_dart_state.h index a5c78c167..988b680be 100644 --- a/lib/ui/ui_dart_state.h +++ b/lib/ui/ui_dart_state.h @@ -100,7 +100,6 @@ class UIDartState : public tonic::DartState { RefPtr font_selector_; fxl::RefPtr skia_unref_queue_; tonic::DartMicrotaskQueue microtask_queue_; - fml::WeakPtrFactory weak_factory_; void AddOrRemoveTaskObserver(bool add); diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 59093da14..a02693c06 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -105,7 +105,7 @@ DartIsolate::DartIsolate(const DartVM* vm, vm->GetSettings().log_tag), vm_(vm), isolate_snapshot_(std::move(isolate_snapshot)), - weak_factory_(this) { + weak_factory_(std::make_unique>(this)) { FXL_DCHECK(isolate_snapshot_) << "Must contain a valid isolate snapshot."; if (vm_ == nullptr) { @@ -464,7 +464,7 @@ bool DartIsolate::Shutdown() { return true; } -static Dart_Isolate DartCreateAndStartServiceIsolate( +Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate( const char* advisory_script_uri, const char* advisory_script_entrypoint, const char* package_root, @@ -493,21 +493,23 @@ static Dart_Isolate DartCreateAndStartServiceIsolate( flags->load_vmservice_library = true; - auto service_isolate = DartIsolate::CreateRootIsolate( - vm.get(), // vm - vm->GetIsolateSnapshot(), // isolate snapshot - null_task_runners, // task runners - nullptr, // window - {}, // resource context - {}, // unref queue - advisory_script_uri == nullptr ? "" : advisory_script_uri, // script uri - advisory_script_entrypoint == nullptr - ? "" - : advisory_script_entrypoint, // script entrypoint - flags // flags - ); - - if (!service_isolate) { + fml::WeakPtr weak_service_isolate = + DartIsolate::CreateRootIsolate( + vm.get(), // vm + vm->GetIsolateSnapshot(), // isolate snapshot + null_task_runners, // task runners + nullptr, // window + {}, // resource context + {}, // unref queue + advisory_script_uri == nullptr ? "" + : advisory_script_uri, // script uri + advisory_script_entrypoint == nullptr + ? "" + : advisory_script_entrypoint, // script entrypoint + flags // flags + ); + + if (!weak_service_isolate) { *error = strdup("Could not create the service isolate."); FXL_DLOG(ERROR) << *error; return nullptr; @@ -516,11 +518,17 @@ static Dart_Isolate DartCreateAndStartServiceIsolate( // The engine never holds a strong reference to the VM service isolate. Since // we are about to lose our last weak reference to it, start the VM service // while we have this reference. + DartIsolate* service_isolate = weak_service_isolate.get(); + + // The service isolate is created and destroyed on arbitrary Dart pool threads + // and can not support a weak pointer factory that must be bound to a specific + // thread. + service_isolate->ResetWeakPtrFactory(); const bool running_from_sources = !DartVM::IsRunningPrecompiledCode() && vm->GetPlatformKernel() == nullptr; - tonic::DartState::Scope scope(service_isolate.get()); + tonic::DartState::Scope scope(service_isolate); if (!DartServiceIsolate::Startup( settings.ipv6 ? "::1" : "127.0.0.1", // server IP address settings.observatory_port, // server observatory port @@ -678,7 +686,13 @@ fxl::RefPtr DartIsolate::GetIsolateSnapshot() const { } fml::WeakPtr DartIsolate::GetWeakIsolatePtr() const { - return weak_factory_.GetWeakPtr(); + return weak_factory_ ? weak_factory_->GetWeakPtr() + : fml::WeakPtr(); +} + +void DartIsolate::ResetWeakPtrFactory() { + FXL_CHECK(weak_factory_); + weak_factory_.reset(); } void DartIsolate::AddIsolateShutdownCallback(fxl::Closure closure) { diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index dd402b157..b78324825 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -103,7 +103,7 @@ class DartIsolate : public UIDartState { Phase phase_ = Phase::Unknown; const fxl::RefPtr isolate_snapshot_; std::vector> shutdown_callbacks_; - fml::WeakPtrFactory weak_factory_; + std::unique_ptr> weak_factory_; FXL_WARN_UNUSED_RESULT bool Initialize(Dart_Isolate isolate, bool is_root_isolate); @@ -116,6 +116,8 @@ class DartIsolate : public UIDartState { FXL_WARN_UNUSED_RESULT bool MarkIsolateRunnable(); + void ResetWeakPtrFactory(); + // |Dart_IsolateCreateCallback| static Dart_Isolate DartIsolateCreateCallback( const char* advisory_script_uri, @@ -126,6 +128,14 @@ class DartIsolate : public UIDartState { DartIsolate* embedder_isolate, char** error); + static Dart_Isolate DartCreateAndStartServiceIsolate( + const char* advisory_script_uri, + const char* advisory_script_entrypoint, + const char* package_root, + const char* package_config, + Dart_IsolateFlags* flags, + char** error); + static std::pair /* embedder */> CreateDartVMAndEmbedderObjectPair(const char* advisory_script_uri, -- GitLab