From e3fb7c26346dae868e1127d9081321e0fdbc2c78 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Thu, 31 Oct 2019 13:40:34 -0700 Subject: [PATCH 1/5] Revert "Revert "Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. (#9888)" (#12327)" This reverts commit 23835f55297f1b6354f089eb0d6f4b6f06f366c3. --- runtime/dart_isolate.cc | 177 +++++++++++++++++++++++++++++++--------- runtime/dart_isolate.h | 35 ++++++-- runtime/dart_vm.cc | 5 ++ 3 files changed, 171 insertions(+), 46 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 719cc681338e7..3911905daaa26 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -66,8 +66,11 @@ std::weak_ptr DartIsolate::CreateRootIsolate( advisory_script_entrypoint, // advisory entrypoint nullptr, // child isolate preparer isolate_create_callback, // isolate create callback - isolate_shutdown_callback // isolate shutdown callback - ))); + isolate_shutdown_callback, // isolate shutdown callback + true, // is_root_isolate + true // is_group_root_isolate + )) + ); std::tie(vm_isolate, embedder_isolate) = CreateDartVMAndEmbedderObjectPair( advisory_script_uri.c_str(), // advisory script URI @@ -110,7 +113,9 @@ DartIsolate::DartIsolate(const Settings& settings, std::string advisory_script_entrypoint, ChildIsolatePreparer child_isolate_preparer, fml::closure isolate_create_callback, - fml::closure isolate_shutdown_callback) + fml::closure isolate_shutdown_callback, + bool is_root_isolate, + bool is_group_root_isolate) : UIDartState(std::move(task_runners), settings.task_observer_add, settings.task_observer_remove, @@ -126,7 +131,9 @@ DartIsolate::DartIsolate(const Settings& settings, isolate_snapshot_(std::move(isolate_snapshot)), child_isolate_preparer_(std::move(child_isolate_preparer)), isolate_create_callback_(isolate_create_callback), - isolate_shutdown_callback_(isolate_shutdown_callback) { + isolate_shutdown_callback_(isolate_shutdown_callback), + is_root_isolate_(is_root_isolate), + is_group_root_isolate_(is_group_root_isolate) { FML_DCHECK(isolate_snapshot_) << "Must contain a valid isolate snapshot."; phase_ = Phase::Uninitialized; } @@ -148,7 +155,7 @@ std::string DartIsolate::GetServiceId() { return service_id; } -bool DartIsolate::Initialize(Dart_Isolate dart_isolate, bool is_root_isolate) { +bool DartIsolate::Initialize(Dart_Isolate dart_isolate) { TRACE_EVENT0("flutter", "DartIsolate::Initialize"); if (phase_ != Phase::Uninitialized) { return false; @@ -162,12 +169,6 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate, bool is_root_isolate) { return false; } - auto* isolate_data = static_cast*>( - Dart_IsolateGroupData(dart_isolate)); - if (isolate_data->get() != this) { - return false; - } - // After this point, isolate scopes can be safely used. SetIsolate(dart_isolate); @@ -179,8 +180,7 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate, bool is_root_isolate) { tonic::DartIsolateScope scope(isolate()); - SetMessageHandlingTaskRunner(GetTaskRunners().GetUITaskRunner(), - is_root_isolate); + SetMessageHandlingTaskRunner(GetTaskRunners().GetUITaskRunner()); if (tonic::LogIfError( Dart_SetLibraryTagHandler(tonic::DartState::HandleLibraryTag))) { @@ -200,9 +200,8 @@ fml::RefPtr DartIsolate::GetMessageHandlingTaskRunner() const { } void DartIsolate::SetMessageHandlingTaskRunner( - fml::RefPtr runner, - bool is_root_isolate) { - if (!is_root_isolate || !runner) { + fml::RefPtr runner) { + if (!IsRootIsolate() || !runner) { return; } @@ -251,7 +250,7 @@ bool DartIsolate::UpdateThreadPoolNames() const { return true; } -bool DartIsolate::LoadLibraries(bool is_root_isolate) { +bool DartIsolate::LoadLibraries() { TRACE_EVENT0("flutter", "DartIsolate::LoadLibraries"); if (phase_ != Phase::Initialized) { return false; @@ -261,11 +260,11 @@ bool DartIsolate::LoadLibraries(bool is_root_isolate) { DartIO::InitForIsolate(); - DartUI::InitForIsolate(is_root_isolate); + DartUI::InitForIsolate(IsRootIsolate()); const bool is_service_isolate = Dart_IsServiceIsolate(isolate()); - DartRuntimeHooks::Install(is_root_isolate && !is_service_isolate, + DartRuntimeHooks::Install(IsRootIsolate() && !is_service_isolate, GetAdvisoryScriptURI()); if (!is_service_isolate) { @@ -645,6 +644,7 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback( Dart_IsolateFlags* flags, std::shared_ptr* parent_embedder_isolate, char** error) { + TRACE_EVENT0("flutter", "DartIsolate::DartIsolateGroupCreateCallback"); if (parent_embedder_isolate == nullptr && strcmp(advisory_script_uri, DART_VM_SERVICE_ISOLATE_NAME) == 0) { // The VM attempts to start the VM service for us on |Dart_Initialize|. In @@ -672,6 +672,56 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback( .first; } +// |Dart_IsolateInitializeCallback| +bool DartIsolate::DartIsolateInitializeCallback(void** child_callback_data, + char** error) { + TRACE_EVENT0("flutter", "DartIsolate::DartIsolateInitializeCallback"); + Dart_Isolate isolate = Dart_CurrentIsolate(); + if (isolate == nullptr) { + *error = strdup("Isolate should be available in initialize callback."); + FML_DLOG(ERROR) << *error; + return false; + } + + auto* root_embedder_isolate = static_cast*>( + Dart_CurrentIsolateGroupData()); + + TaskRunners null_task_runners( + (*root_embedder_isolate)->GetAdvisoryScriptURI(), + /* platform= */ nullptr, /* gpu= */ nullptr, + /* ui= */ nullptr, + /* io= */ nullptr); + + auto embedder_isolate = std::make_unique>( + std::shared_ptr(new DartIsolate( + (*root_embedder_isolate)->GetSettings(), // settings + (*root_embedder_isolate)->GetIsolateSnapshot(), // isolate_snapshot + null_task_runners, // task_runners + fml::WeakPtr{}, // io_manager + fml::RefPtr{}, // unref_queue + fml::WeakPtr{}, // image_decoder + (*root_embedder_isolate)->GetAdvisoryScriptURI(),// advisory_script_uri + (*root_embedder_isolate)->GetAdvisoryScriptEntrypoint(), // advisory_script_entrypoint + (*root_embedder_isolate)->child_isolate_preparer_, // preparer + (*root_embedder_isolate)->isolate_create_callback_, // on create + (*root_embedder_isolate)->isolate_shutdown_callback_, // on shutdown + false, // is_root_isolate + false))); // is_group_root_isolate + + // root isolate should have been created via CreateRootIsolate and + // CreateDartVMAndEmbedderObjectPair + if (!InitializeIsolate(*embedder_isolate, isolate, error)) { + return false; + } + + // The ownership of the embedder object is controlled by the Dart VM. So the + // only reference returned to the caller is weak. + *child_callback_data = embedder_isolate.release(); + + Dart_EnterIsolate(isolate); + return true; +} + std::pair> DartIsolate::CreateDartVMAndEmbedderObjectPair( const char* advisory_script_uri, @@ -711,11 +761,11 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair( fml::WeakPtr{}, // image_decoder advisory_script_uri, // advisory_script_uri advisory_script_entrypoint, // advisory_script_entrypoint - (*raw_embedder_isolate)->child_isolate_preparer_, // preparer - (*raw_embedder_isolate)->isolate_create_callback_, // on create - (*raw_embedder_isolate)->isolate_shutdown_callback_ // on shutdown - )) - + (*raw_embedder_isolate)->child_isolate_preparer_, // preparer + (*raw_embedder_isolate)->isolate_create_callback_, // on create + (*raw_embedder_isolate)->isolate_shutdown_callback_, // on shutdown + is_root_isolate, + true)) // is_root_group_isolate ); } @@ -735,38 +785,53 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair( return {nullptr, {}}; } - if (!(*embedder_isolate)->Initialize(isolate, is_root_isolate)) { + if (!InitializeIsolate(*embedder_isolate, isolate, error)) { + return {nullptr, {}}; + } + + auto* isolate_data = static_cast*>( + Dart_IsolateGroupData(isolate)); + FML_DCHECK(isolate_data->get() == embedder_isolate->get()); + + auto weak_embedder_isolate = (*embedder_isolate)->GetWeakIsolatePtr(); + + // The ownership of the embedder object is controlled by the Dart VM. So the + // only reference returned to the caller is weak. + embedder_isolate.release(); + return {isolate, weak_embedder_isolate}; +} + +bool DartIsolate::InitializeIsolate( + std::shared_ptr embedder_isolate, + Dart_Isolate isolate, + char** error) { + TRACE_EVENT0("flutter", "DartIsolate::InitializeIsolate"); + if (!embedder_isolate->Initialize(isolate)) { *error = strdup("Embedder could not initialize the Dart isolate."); FML_DLOG(ERROR) << *error; - return {nullptr, {}}; + return false; } - if (!(*embedder_isolate)->LoadLibraries(is_root_isolate)) { + if (!embedder_isolate->LoadLibraries()) { *error = strdup("Embedder could not load libraries in the new Dart isolate."); FML_DLOG(ERROR) << *error; - return {nullptr, {}}; + return false; } - auto weak_embedder_isolate = (*embedder_isolate)->GetWeakIsolatePtr(); - // Root isolates will be setup by the engine and the service isolate (which is // also a root isolate) by the utility routines in the VM. However, secondary // isolates will be run by the VM if they are marked as runnable. - if (!is_root_isolate) { - FML_DCHECK((*embedder_isolate)->child_isolate_preparer_); - if (!(*embedder_isolate) - ->child_isolate_preparer_((*embedder_isolate).get())) { + if (!embedder_isolate->IsRootIsolate()) { + FML_DCHECK(embedder_isolate->child_isolate_preparer_); + if (!embedder_isolate->child_isolate_preparer_(embedder_isolate.get())) { *error = strdup("Could not prepare the child isolate to run."); FML_DLOG(ERROR) << *error; - return {nullptr, {}}; + return false; } } - // The ownership of the embedder object is controlled by the Dart VM. So the - // only reference returned to the caller is weak. - embedder_isolate.release(); - return {isolate, weak_embedder_isolate}; + return true; } // |Dart_IsolateShutdownCallback| @@ -783,6 +848,40 @@ void DartIsolate::DartIsolateShutdownCallback( // |Dart_IsolateGroupCleanupCallback| void DartIsolate::DartIsolateGroupCleanupCallback( std::shared_ptr* isolate_data) { + TRACE_EVENT0("flutter", "DartIsolate::DartIsolateGroupCleanupCallback"); + FML_DLOG(INFO) << "DartIsolateGroupCleanupCallback isolate_data " + << isolate_data; + + delete isolate_data; +} + +// |Dart_IsolateCleanupCallback| +void DartIsolate::DartIsolateCleanupCallback( + std::shared_ptr* isolate_group_data, + std::shared_ptr* isolate_data) { + TRACE_EVENT0("flutter", "DartIsolate::DartIsolateCleanupCallback"); + + if ((*isolate_data)->IsRootIsolate()) { + // isolate_data will be cleaned up as part of IsolateGroup cleanup + FML_DLOG(INFO) + << "DartIsolateCleanupCallback no-op for root isolate isolate_data " + << isolate_data; + return; + } + if ((*isolate_data)->IsGroupRootIsolate()) { + // Even if isolate was not a root isolate(i.e. was spawned), + // it might have IsolateGroup created for it (when + // --no-enable-isolate-groups dart vm flag is used). + // Then its isolate_data will be cleaned up as part of IsolateGroup + // cleanup as well. + FML_DLOG(INFO) << "DartIsolateCleanupCallback no-op for group root isolate " + "isolate_data " + << isolate_data; + return; + } + + FML_DLOG(INFO) << "DartIsolateCleanupCallback cleaned up isolate_data " + << isolate_data; delete isolate_data; } diff --git a/runtime/dart_isolate.h b/runtime/dart_isolate.h index 7ee76ded30501..e7f35b238f508 100644 --- a/runtime/dart_isolate.h +++ b/runtime/dart_isolate.h @@ -399,6 +399,13 @@ class DartIsolate : public UIDartState { /// fml::RefPtr GetMessageHandlingTaskRunner() const; + // Root isolate of the VM application + bool IsRootIsolate() const { return is_root_isolate_; } + // Isolate that owns IsolateGroup it lives in. + // When --no-enable-isolate-groups dart vm flag is set, + // all child isolates will have their own IsolateGroups. + bool IsGroupRootIsolate() const { return is_group_root_isolate_; } + private: using ChildIsolatePreparer = std::function; @@ -423,6 +430,8 @@ class DartIsolate : public UIDartState { fml::RefPtr message_handling_task_runner_; const fml::closure isolate_create_callback_; const fml::closure isolate_shutdown_callback_; + const bool is_root_isolate_; + const bool is_group_root_isolate_; DartIsolate(const Settings& settings, fml::RefPtr isolate_snapshot, @@ -434,18 +443,17 @@ class DartIsolate : public UIDartState { std::string advisory_script_entrypoint, ChildIsolatePreparer child_isolate_preparer, fml::closure isolate_create_callback, - fml::closure isolate_shutdown_callback); - - FML_WARN_UNUSED_RESULT bool Initialize(Dart_Isolate isolate, - bool is_root_isolate); + fml::closure isolate_shutdown_callback, + bool is_root_isolate, + bool is_group_root_isolate); + FML_WARN_UNUSED_RESULT bool Initialize(Dart_Isolate isolate); - void SetMessageHandlingTaskRunner(fml::RefPtr runner, - bool is_root_isolate); + void SetMessageHandlingTaskRunner(fml::RefPtr runner); bool LoadKernel(std::shared_ptr mapping, bool last_piece); FML_WARN_UNUSED_RESULT - bool LoadLibraries(bool is_root_isolate); + bool LoadLibraries(); bool UpdateThreadPoolNames() const; @@ -464,6 +472,10 @@ class DartIsolate : public UIDartState { std::shared_ptr* embedder_isolate, char** error); + // |Dart_IsolateInitializeCallback| + static bool DartIsolateInitializeCallback(void** child_callback_data, + char** error); + static Dart_Isolate DartCreateAndStartServiceIsolate( const char* package_root, const char* package_config, @@ -482,11 +494,20 @@ class DartIsolate : public UIDartState { bool is_root_isolate, char** error); + static bool InitializeIsolate(std::shared_ptr embedder_isolate, + Dart_Isolate isolate, + char** error); + // |Dart_IsolateShutdownCallback| static void DartIsolateShutdownCallback( std::shared_ptr* isolate_group_data, std::shared_ptr* isolate_data); + // |Dart_IsolateCleanupCallback| + static void DartIsolateCleanupCallback( + std::shared_ptr* isolate_group_data, + std::shared_ptr* isolate_data); + // |Dart_IsolateGroupCleanupCallback| static void DartIsolateGroupCleanupCallback( std::shared_ptr* isolate_group_data); diff --git a/runtime/dart_vm.cc b/runtime/dart_vm.cc index de9d08e3b3b67..5e92b8ead754b 100644 --- a/runtime/dart_vm.cc +++ b/runtime/dart_vm.cc @@ -392,9 +392,14 @@ DartVM::DartVM(std::shared_ptr vm_data, vm_data_->GetVMSnapshot().GetInstructionsMapping(); params.create_group = reinterpret_cast( DartIsolate::DartIsolateGroupCreateCallback); + params.initialize_isolate = + reinterpret_cast( + DartIsolate::DartIsolateInitializeCallback); params.shutdown_isolate = reinterpret_cast( DartIsolate::DartIsolateShutdownCallback); + params.cleanup_isolate = reinterpret_cast( + DartIsolate::DartIsolateCleanupCallback); params.cleanup_group = reinterpret_cast( DartIsolate::DartIsolateGroupCleanupCallback); params.thread_exit = ThreadExitCallback; From b96df26972d1fff1073011d1502f632ad1248c58 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Fri, 8 Nov 2019 13:01:50 -0800 Subject: [PATCH 2/5] Ensure that when isolate shuts down it calls isolate_data, rather than isolage_group_data callback. When isolates share same isolate group, it's important that when they shut down, relevant shutdown callback is invoked. --- runtime/dart_isolate.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 8966eefd23f22..054462a052afd 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -848,7 +848,7 @@ void DartIsolate::DartIsolateShutdownCallback( FML_DLOG(INFO) << "DartIsolateShutdownCallback" << " isolate_group_data " << isolate_group_data << " isolate_data " << isolate_data; - isolate_group_data->get()->OnShutdownCallback(); + isolate_data->get()->OnShutdownCallback(); } // |Dart_IsolateGroupCleanupCallback| From 90e3e267d8ec127839df88a8a069dc51ac2c7f88 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Fri, 8 Nov 2019 13:17:10 -0800 Subject: [PATCH 3/5] /ci/format.sh --- runtime/dart_isolate.cc | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 054462a052afd..cc306d5aae7dc 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -57,22 +57,21 @@ std::weak_ptr DartIsolate::CreateRootIsolate( // being prepared to run. auto root_embedder_data = std::make_unique>( std::shared_ptr(new DartIsolate( - settings, // settings - std::move(isolate_snapshot), // isolate snapshot - task_runners, // task runners + settings, // settings + std::move(isolate_snapshot), // isolate snapshot + task_runners, // task runners std::move(snapshot_delegate), // snapshot delegate - std::move(io_manager), // IO manager - std::move(unref_queue), // Skia unref queue - std::move(image_decoder), // Image Decoder - advisory_script_uri, // advisory URI - advisory_script_entrypoint, // advisory entrypoint - nullptr, // child isolate preparer - isolate_create_callback, // isolate create callback - isolate_shutdown_callback, // isolate shutdown callback - true, // is_root_isolate - true // is_group_root_isolate - )) - ); + std::move(io_manager), // IO manager + std::move(unref_queue), // Skia unref queue + std::move(image_decoder), // Image Decoder + advisory_script_uri, // advisory URI + advisory_script_entrypoint, // advisory entrypoint + nullptr, // child isolate preparer + isolate_create_callback, // isolate create callback + isolate_shutdown_callback, // isolate shutdown callback + true, // is_root_isolate + true // is_group_root_isolate + ))); std::tie(vm_isolate, embedder_isolate) = CreateDartVMAndEmbedderObjectPair( advisory_script_uri.c_str(), // advisory script URI @@ -705,12 +704,14 @@ bool DartIsolate::DartIsolateInitializeCallback(void** child_callback_data, fml::WeakPtr{}, // io_manager fml::RefPtr{}, // unref_queue fml::WeakPtr{}, // image_decoder - (*root_embedder_isolate)->GetAdvisoryScriptURI(),// advisory_script_uri - (*root_embedder_isolate)->GetAdvisoryScriptEntrypoint(), // advisory_script_entrypoint + (*root_embedder_isolate) + ->GetAdvisoryScriptURI(), // advisory_script_uri + (*root_embedder_isolate) + ->GetAdvisoryScriptEntrypoint(), // advisory_script_entrypoint (*root_embedder_isolate)->child_isolate_preparer_, // preparer (*root_embedder_isolate)->isolate_create_callback_, // on create (*root_embedder_isolate)->isolate_shutdown_callback_, // on shutdown - false, // is_root_isolate + false, // is_root_isolate false))); // is_group_root_isolate // root isolate should have been created via CreateRootIsolate and From cc331b61ac0cf2275d44ecebb437f0b20f77120f Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Fri, 8 Nov 2019 13:28:59 -0800 Subject: [PATCH 4/5] Fix merge issue --- runtime/dart_isolate.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index cc306d5aae7dc..f6bc1f695ed5f 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -701,6 +701,7 @@ bool DartIsolate::DartIsolateInitializeCallback(void** child_callback_data, (*root_embedder_isolate)->GetSettings(), // settings (*root_embedder_isolate)->GetIsolateSnapshot(), // isolate_snapshot null_task_runners, // task_runners + fml::WeakPtr{}, // snapshot_delegate fml::WeakPtr{}, // io_manager fml::RefPtr{}, // unref_queue fml::WeakPtr{}, // image_decoder From 635f4a91318fa8336847dd36b18153399aeb8f88 Mon Sep 17 00:00:00 2001 From: Alexander Aprelev Date: Tue, 12 Nov 2019 09:30:14 -0800 Subject: [PATCH 5/5] Update test to check for correct shutdown callback invocation. --- runtime/dart_isolate_unittests.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 5dd39c5ebcc87..33b60ae26eefc 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -364,6 +364,7 @@ TEST_F(DartIsolateTest, CanSaveCompilationTrace) { TEST_F(DartIsolateTest, CanLaunchSecondaryIsolates) { fml::CountDownLatch latch(3); fml::AutoResetWaitableEvent child_shutdown_latch; + fml::AutoResetWaitableEvent root_isolate_shutdown_latch; AddNativeCallback("NotifyNative", CREATE_NATIVE_ENTRY(([&latch](Dart_NativeArguments args) { latch.CountDown(); @@ -376,6 +377,9 @@ TEST_F(DartIsolateTest, CanLaunchSecondaryIsolates) { latch.CountDown(); }))); auto settings = CreateSettingsForFixture(); + settings.root_isolate_shutdown_callback = [&root_isolate_shutdown_latch]() { + root_isolate_shutdown_latch.Signal(); + }; settings.isolate_shutdown_callback = [&child_shutdown_latch]() { child_shutdown_latch.Signal(); }; @@ -385,6 +389,7 @@ TEST_F(DartIsolateTest, CanLaunchSecondaryIsolates) { ASSERT_TRUE(isolate); ASSERT_EQ(isolate->get()->GetPhase(), DartIsolate::Phase::Running); child_shutdown_latch.Wait(); // wait for child isolate to shutdown first + ASSERT_FALSE(root_isolate_shutdown_latch.IsSignaledForTest()); latch.Wait(); // wait for last NotifyNative called by main isolate // root isolate will be auto-shutdown }