Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. #9888

Merged
aam merged 13 commits into from
Sep 4, 2019
Merged

Provide dart vm initalize isolate callback so that children isolates belong to parent's isolate group. #9888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
de4db7f
Provide dart vm initalize isolate callback so that children isolates …
aam Jul 17, 2019
5a545f8
Fix formatting, add traces
aam Jul 17, 2019
0819251
Merge branch 'master' into provide-initialize-isolate-callback
aam Aug 9, 2019
5a8ea6d
Cleanup isolate data
aam Aug 12, 2019
4905403
Igore isolate shutdown for service isolate
aam Aug 12, 2019
8553cbf
Reformat
aam Aug 12, 2019
e40a253
window() check is not reliable, introduce is_root_isolate property
aam Aug 12, 2019
48150b3
Merge branch 'master' into provide-initialize-isolate-callback
aam Aug 27, 2019
553407e
Introduce is_group_root_isolate, fix DartIsolateCleanupCallback type
aam Aug 28, 2019
d7fb798
Merge branch 'master' into provide-initialize-isolate-callback
aam Aug 29, 2019
6bfaa05
Run ci/format.sh
aam Aug 29, 2019
8dbd38c
Merge branch 'master' into provide-initialize-isolate-callback
aam Sep 4, 2019
0dfecf8
Get rid of consts, introduce FML_DCHECK
aam Sep 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 139 additions & 39 deletions runtime/dart_isolate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ std::weak_ptr<DartIsolate> 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(
Expand Down Expand Up @@ -110,7 +112,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,
Expand All @@ -126,7 +130,9 @@ DartIsolate::DartIsolate(const Settings& settings,
shared_snapshot_(std::move(shared_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;
}
Expand All @@ -148,7 +154,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;
Expand All @@ -162,12 +168,6 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate, bool is_root_isolate) {
return false;
}

auto* isolate_data = static_cast<std::shared_ptr<DartIsolate>*>(
Dart_IsolateGroupData(dart_isolate));
if (isolate_data->get() != this) {
return false;
}

// After this point, isolate scopes can be safely used.
SetIsolate(dart_isolate);

Expand All @@ -179,8 +179,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))) {
Expand All @@ -200,9 +199,8 @@ fml::RefPtr<fml::TaskRunner> DartIsolate::GetMessageHandlingTaskRunner() const {
}

void DartIsolate::SetMessageHandlingTaskRunner(
fml::RefPtr<fml::TaskRunner> runner,
bool is_root_isolate) {
if (!is_root_isolate || !runner) {
fml::RefPtr<fml::TaskRunner> runner) {
if (!IsRootIsolate() || !runner) {
return;
}

Expand Down Expand Up @@ -251,7 +249,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;
Expand All @@ -261,11 +259,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) {
Expand Down Expand Up @@ -645,6 +643,7 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback(
Dart_IsolateFlags* flags,
std::shared_ptr<DartIsolate>* parent_embedder_isolate,
char** error) {
TRACE_EVENT0("flutter", "DartIsolate::DartIsolateGroupCreateCallback");
Copy link
Member

@mkustermann mkustermann Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I cannot comment on the line blow due to github review, but this comment belongs to line 664)

The code below should be unreachable or should error if there is no support for Isolate.spawnUri, since every Isolate.spawn will cause us to use the initialize_isolate callback. The only other isolate groups created are for service/kernel isolate.

That being said, I don't know how the "multiple flutter engine shell" feature works, though I assume they will not use this code either.

Therefore I think the call to CreateDartVMAndEmbedderObjectPair(..., is_root_isolate=false) should be replaced with an error (or unreachable).

Copy link
Member Author

@aam aam Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. This DartIsolateGroupCreateCallback is called on isolate group creation. That will be called two times(service isolate group, main isolate group) and for main isolate the call CreateDartVMAndEmbedderObjectPair(..., is_root_isolate=false) will be triggered, right?

Copy link
Member

@mkustermann mkustermann Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DartIsolateGroupCreateCallback is called from the VM, if the VM wants to create new isolate groups. This happens in these cases:

  • VM wants to create service-isolate
  • Isolate.spawn (only if isolate groups are disabled, if they are enabled then only initialize_isolate callback is used, not create_group)
  • Isolate.spawnUri (not supported by flutter I suppose)
  • VM wants to create kernel-isolate (not used by flutter I suppose)

=> If isolate groups are enabled, the only thing this callback will ever get to do is starting the service isolate.

The actual main UI isolate is created by the embedder itself and doesn't go through any callbacks. I believe this is done in runtime/runtime_controller.cc

That being said, I think you can ignore my original comment: As discussed offline we want to have the flexibility to flip --enable-isolate-groups on/off for some time, so we keep this code.

Copy link
Member Author

@aam aam Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for detailed explanation, makes sense.

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
Expand Down Expand Up @@ -672,6 +671,58 @@ Dart_Isolate DartIsolate::DartIsolateGroupCreateCallback(
.first;
Copy link
Member

@mkustermann mkustermann Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When falling back to --no-enable-isolate-groups line 665 passes /*is_root_isolate=*/false. This can cause a double free (I think):

  • Isolate.spawn() creates a new isolate, invoking the code on line 658.
  • That isolate dies at some point
  • The cleanup_isolate callback is called. Since !isolate_data->get()->IsRootIsolate() we free the DartIsolate object.
  • The cleanup_group callback is called. We delete the same object again

The reason this happens is that, is_root_isolate is different from is_group_isolate if --no-enable-isolate-groups. (Maybe an extra boolean is_isolate_group_root could be used for the memory management)

Copy link
Member Author

@aam aam Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

// |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<std::shared_ptr<DartIsolate>*>(
Dart_CurrentIsolateGroupData());
Copy link
Member

@mkustermann mkustermann Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a static_cast?

raw_embedder_isolate -> root_embedder_isolate?

Copy link
Member Author

@aam aam Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


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<DartIsolate>>(
std::make_shared<DartIsolate>(
(*root_embedder_isolate)->GetSettings(), // settings
(*root_embedder_isolate)->GetIsolateSnapshot(), // isolate_snapshot
(*root_embedder_isolate)->GetSharedSnapshot(), // shared_snapshot
null_task_runners, // task_runners
fml::WeakPtr<IOManager>{}, // io_manager
fml::WeakPtr<ImageDecoder>{}, // io_manager
(*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();
Copy link
Member

@mkustermann mkustermann Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding the same comment as below regarding the release()

Copy link
Member Author

@aam aam Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


Dart_EnterIsolate(isolate);
return true;
}

std::pair<Dart_Isolate, std::weak_ptr<DartIsolate>>
DartIsolate::CreateDartVMAndEmbedderObjectPair(
const char* advisory_script_uri,
Expand Down Expand Up @@ -711,12 +762,11 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair(
fml::WeakPtr<ImageDecoder>{}, // io_manager
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
}

// Create the Dart VM isolate and give it the embedder object as the baton.
Expand All @@ -736,50 +786,100 @@ 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<std::shared_ptr<DartIsolate>*>(
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<DartIsolate> 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|
void DartIsolate::DartIsolateShutdownCallback(
std::shared_ptr<DartIsolate>* isolate_group_data,
std::shared_ptr<DartIsolate>* isolate_data) {
TRACE_EVENT0("flutter", "DartIsolate::DartIsolateShutdownCallback");
isolate_group_data->get()->OnShutdownCallback();
}

// |Dart_IsolateGroupCleanupCallback|
void DartIsolate::DartIsolateGroupCleanupCallback(
std::shared_ptr<DartIsolate>* 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<DartIsolate>* isolate_group_data,
std::shared_ptr<DartIsolate>* 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;
}

Expand Down
35 changes: 29 additions & 6 deletions runtime/dart_isolate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ class DartIsolate : public UIDartState {
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);

~DartIsolate() override;

Expand Down Expand Up @@ -113,6 +115,13 @@ class DartIsolate : public UIDartState {

fml::RefPtr<fml::TaskRunner> 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:
bool LoadKernel(std::shared_ptr<const fml::Mapping> mapping, bool last_piece);

Expand All @@ -139,14 +148,15 @@ class DartIsolate : public UIDartState {
const fml::closure isolate_create_callback_;
const fml::closure isolate_shutdown_callback_;

FML_WARN_UNUSED_RESULT bool Initialize(Dart_Isolate isolate,
bool is_root_isolate);
const bool is_root_isolate_;
const bool is_group_root_isolate_;

FML_WARN_UNUSED_RESULT bool Initialize(Dart_Isolate isolate);

void SetMessageHandlingTaskRunner(fml::RefPtr<fml::TaskRunner> runner,
bool is_root_isolate);
void SetMessageHandlingTaskRunner(fml::RefPtr<fml::TaskRunner> runner);

FML_WARN_UNUSED_RESULT
bool LoadLibraries(bool is_root_isolate);
bool LoadLibraries();

bool UpdateThreadPoolNames() const;

Expand All @@ -165,6 +175,10 @@ class DartIsolate : public UIDartState {
std::shared_ptr<DartIsolate>* 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,
Expand All @@ -183,11 +197,20 @@ class DartIsolate : public UIDartState {
bool is_root_isolate,
char** error);

static bool InitializeIsolate(std::shared_ptr<DartIsolate> embedder_isolate,
Dart_Isolate isolate,
char** error);

// |Dart_IsolateShutdownCallback|
static void DartIsolateShutdownCallback(
std::shared_ptr<DartIsolate>* isolate_group_data,
std::shared_ptr<DartIsolate>* isolate_data);

// |Dart_IsolateCleanupCallback|
static void DartIsolateCleanupCallback(
std::shared_ptr<DartIsolate>* isolate_group_data,
std::shared_ptr<DartIsolate>* isolate_data);

// |Dart_IsolateGroupCleanupCallback|
static void DartIsolateGroupCleanupCallback(
std::shared_ptr<DartIsolate>* isolate_group_data);
Expand Down
5 changes: 5 additions & 0 deletions runtime/dart_vm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,9 +395,14 @@ DartVM::DartVM(std::shared_ptr<const DartVMData> vm_data,
vm_data_->GetVMSnapshot().GetInstructionsMapping();
params.create_group = reinterpret_cast<decltype(params.create_group)>(
DartIsolate::DartIsolateGroupCreateCallback);
params.initialize_isolate =
reinterpret_cast<decltype(params.initialize_isolate)>(
DartIsolate::DartIsolateInitializeCallback);
params.shutdown_isolate =
reinterpret_cast<decltype(params.shutdown_isolate)>(
DartIsolate::DartIsolateShutdownCallback);
params.cleanup_isolate = reinterpret_cast<decltype(params.cleanup_isolate)>(
DartIsolate::DartIsolateCleanupCallback);
params.cleanup_group = reinterpret_cast<decltype(params.cleanup_group)>(
DartIsolate::DartIsolateGroupCleanupCallback);
params.thread_exit = ThreadExitCallback;
Expand Down