Skip to content

Commit

Permalink
Separate the data required to bootstrap the VM into its own class. (f…
Browse files Browse the repository at this point in the history 
…lutter#8397)

When attempting to shutdown and subsequently restart the VM, having the
VM own this data introduces lifecycle issues due to circular references.
  • Loading branch information
@chinmaygarde
chinmaygarde authored Apr 1, 2019
1 parent 8c9aca4 commit c991647
Show file tree
Hide file tree
Showing 24 changed files with 632 additions and 290 deletions.
4 changes: 4 additions & 0 deletions ci/licenses_golden/licenses_flutter
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,10 @@ FILE: ../../../flutter/runtime/dart_snapshot_buffer.cc
FILE: ../../../flutter/runtime/dart_snapshot_buffer.h
FILE: ../../../flutter/runtime/dart_vm.cc
FILE: ../../../flutter/runtime/dart_vm.h
FILE: ../../../flutter/runtime/dart_vm_data.cc
FILE: ../../../flutter/runtime/dart_vm_data.h
FILE: ../../../flutter/runtime/dart_vm_lifecycle.cc
FILE: ../../../flutter/runtime/dart_vm_lifecycle.h
FILE: ../../../flutter/runtime/dart_vm_unittests.cc
FILE: ../../../flutter/runtime/embedder_resources.cc
FILE: ../../../flutter/runtime/embedder_resources.h
Expand Down
2 changes: 2 additions & 0 deletions common/settings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ struct Settings {
// Font settings
bool use_test_fonts = false;

bool leak_vm = true;

// Engine settings
TaskObserverAdd task_observer_add;
TaskObserverRemove task_observer_remove;
Expand Down
18 changes: 12 additions & 6 deletions lib/ui/isolate_name_server/isolate_name_server_natives.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ namespace blink {

Dart_Handle IsolateNameServerNatives::LookupPortByName(
const std::string& name) {
IsolateNameServer* name_server =
UIDartState::Current()->GetIsolateNameServer();
auto name_server = UIDartState::Current()->GetIsolateNameServer();
if (!name_server) {
return Dart_Null();
}
Dart_Port port = name_server->LookupIsolatePortByName(name);
if (port == ILLEGAL_PORT) {
return Dart_Null();
Expand All @@ -26,8 +28,10 @@ Dart_Handle IsolateNameServerNatives::LookupPortByName(
Dart_Handle IsolateNameServerNatives::RegisterPortWithName(
Dart_Handle port_handle,
const std::string& name) {
IsolateNameServer* name_server =
UIDartState::Current()->GetIsolateNameServer();
auto name_server = UIDartState::Current()->GetIsolateNameServer();
if (!name_server) {
return Dart_False();
}
Dart_Port port = ILLEGAL_PORT;
Dart_SendPortGetId(port_handle, &port);
if (!name_server->RegisterIsolatePortWithName(port, name)) {
Expand All @@ -38,8 +42,10 @@ Dart_Handle IsolateNameServerNatives::RegisterPortWithName(

Dart_Handle IsolateNameServerNatives::RemovePortNameMapping(
const std::string& name) {
IsolateNameServer* name_server =
UIDartState::Current()->GetIsolateNameServer();
auto name_server = UIDartState::Current()->GetIsolateNameServer();
if (!name_server) {
return Dart_False();
}
if (!name_server->RemoveIsolateNameMapping(name)) {
return Dart_False();
}
Expand Down
6 changes: 3 additions & 3 deletions lib/ui/ui_dart_state.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ UIDartState::UIDartState(
std::string advisory_script_entrypoint,
std::string logger_prefix,
UnhandledExceptionCallback unhandled_exception_callback,
IsolateNameServer* isolate_name_server)
std::shared_ptr<IsolateNameServer> isolate_name_server)
: task_runners_(std::move(task_runners)),
add_callback_(std::move(add_callback)),
remove_callback_(std::move(remove_callback)),
Expand All @@ -33,7 +33,7 @@ UIDartState::UIDartState(
advisory_script_entrypoint_(std::move(advisory_script_entrypoint)),
logger_prefix_(std::move(logger_prefix)),
unhandled_exception_callback_(unhandled_exception_callback),
isolate_name_server_(isolate_name_server) {
isolate_name_server_(std::move(isolate_name_server)) {
AddOrRemoveTaskObserver(true /* add */);
}

Expand Down Expand Up @@ -124,7 +124,7 @@ fml::WeakPtr<GrContext> UIDartState::GetResourceContext() const {
return io_manager_->GetResourceContext();
}

IsolateNameServer* UIDartState::GetIsolateNameServer() {
std::shared_ptr<IsolateNameServer> UIDartState::GetIsolateNameServer() const {
return isolate_name_server_;
}

Expand Down
6 changes: 3 additions & 3 deletions lib/ui/ui_dart_state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class UIDartState : public tonic::DartState {

fml::WeakPtr<GrContext> GetResourceContext() const;

IsolateNameServer* GetIsolateNameServer();
std::shared_ptr<IsolateNameServer> GetIsolateNameServer() const;

tonic::DartErrorHandleType GetLastError();

Expand Down Expand Up @@ -81,7 +81,7 @@ class UIDartState : public tonic::DartState {
std::string advisory_script_entrypoint,
std::string logger_prefix,
UnhandledExceptionCallback unhandled_exception_callback,
IsolateNameServer* isolate_name_server);
std::shared_ptr<IsolateNameServer> isolate_name_server);

~UIDartState() override;

Expand All @@ -107,7 +107,7 @@ class UIDartState : public tonic::DartState {
std::unique_ptr<Window> window_;
tonic::DartMicrotaskQueue microtask_queue_;
UnhandledExceptionCallback unhandled_exception_callback_;
IsolateNameServer* isolate_name_server_;
const std::shared_ptr<IsolateNameServer> isolate_name_server_;

void AddOrRemoveTaskObserver(bool add);
};
Expand Down
5 changes: 5 additions & 0 deletions runtime/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ source_set("runtime") {
"dart_snapshot_buffer.h",
"dart_vm.cc",
"dart_vm.h",
"dart_vm_data.cc",
"dart_vm_data.h",
"dart_vm_lifecycle.cc",
"dart_vm_lifecycle.h",
"embedder_resources.cc",
"embedder_resources.h",
"runtime_controller.cc",
Expand Down Expand Up @@ -115,6 +119,7 @@ executable("runtime_unittests") {
"$flutter_root/common",
"$flutter_root/fml",
"$flutter_root/lib/snapshot",
"$flutter_root/shell/common",
"$flutter_root/testing:dart",
"//third_party/skia",
"//third_party/tonic",
Expand Down
89 changes: 44 additions & 45 deletions runtime/dart_isolate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "flutter/lib/ui/dart_ui.h"
#include "flutter/runtime/dart_service_isolate.h"
#include "flutter/runtime/dart_vm.h"
#include "flutter/runtime/dart_vm_lifecycle.h"
#include "third_party/dart/runtime/include/dart_api.h"
#include "third_party/dart/runtime/include/dart_tools_api.h"
#include "third_party/tonic/converter/dart_converter.h"
Expand All @@ -29,9 +30,9 @@
namespace blink {

std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
DartVM* vm,
fml::RefPtr<DartSnapshot> isolate_snapshot,
fml::RefPtr<DartSnapshot> shared_snapshot,
const Settings& settings,
fml::RefPtr<const DartSnapshot> isolate_snapshot,
fml::RefPtr<const DartSnapshot> shared_snapshot,
TaskRunners task_runners,
std::unique_ptr<Window> window,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
Expand All @@ -50,7 +51,7 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
// isolate lifecycle is entirely managed by the VM).
auto root_embedder_data = std::make_unique<std::shared_ptr<DartIsolate>>(
std::make_shared<DartIsolate>(
vm, // VM
settings, // settings
std::move(isolate_snapshot), // isolate snapshot
std::move(shared_snapshot), // shared snapshot
task_runners, // task runners
Expand Down Expand Up @@ -93,46 +94,41 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRootIsolate(
return embedder_isolate;
}

DartIsolate::DartIsolate(DartVM* vm,
fml::RefPtr<DartSnapshot> isolate_snapshot,
fml::RefPtr<DartSnapshot> shared_snapshot,
DartIsolate::DartIsolate(const Settings& settings,
fml::RefPtr<const DartSnapshot> isolate_snapshot,
fml::RefPtr<const DartSnapshot> shared_snapshot,
TaskRunners task_runners,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
fml::WeakPtr<IOManager> io_manager,
std::string advisory_script_uri,
std::string advisory_script_entrypoint,
ChildIsolatePreparer child_isolate_preparer)
: UIDartState(std::move(task_runners),
vm->GetSettings().task_observer_add,
vm->GetSettings().task_observer_remove,
settings.task_observer_add,
settings.task_observer_remove,
std::move(snapshot_delegate),
std::move(io_manager),
advisory_script_uri,
advisory_script_entrypoint,
vm->GetSettings().log_tag,
vm->GetSettings().unhandled_exception_callback,
vm->GetIsolateNameServer()),
vm_(vm),
settings.log_tag,
settings.unhandled_exception_callback,
DartVMRef::GetIsolateNameServer()),
settings_(settings),
isolate_snapshot_(std::move(isolate_snapshot)),
shared_snapshot_(std::move(shared_snapshot)),
child_isolate_preparer_(std::move(child_isolate_preparer)) {
FML_DCHECK(isolate_snapshot_) << "Must contain a valid isolate snapshot.";

if (vm_ == nullptr) {
return;
}

phase_ = Phase::Uninitialized;
}

DartIsolate::~DartIsolate() = default;

DartIsolate::Phase DartIsolate::GetPhase() const {
return phase_;
const Settings& DartIsolate::GetSettings() const {
return settings_;
}

DartVM* DartIsolate::GetDartVM() const {
return vm_;
DartIsolate::Phase DartIsolate::GetPhase() const {
return phase_;
}

bool DartIsolate::Initialize(Dart_Isolate dart_isolate, bool is_root_isolate) {
Expand Down Expand Up @@ -500,16 +496,16 @@ Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate(
const char* package_config,
Dart_IsolateFlags* flags,
char** error) {
auto vm = DartVM::ForProcessIfInitialized();
auto vm_data = DartVMRef::GetVMData();

if (!vm) {
if (!vm_data) {
*error = strdup(
"Could not resolve the VM when attempting to create the service "
"isolate.");
"Could not access VM data to initialize isolates. This may be because "
"the VM has initialized shutdown on another thread already.");
return nullptr;
}

const auto& settings = vm->GetSettings();
const auto& settings = vm_data->GetSettings();

if (!settings.enable_observatory) {
FML_DLOG(INFO) << "Observatory is disabled.";
Expand All @@ -524,16 +520,16 @@ Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate(

std::weak_ptr<DartIsolate> weak_service_isolate =
DartIsolate::CreateRootIsolate(
vm.get(), // vm
vm->GetIsolateSnapshot(), // isolate snapshot
vm->GetSharedSnapshot(), // shared snapshot
null_task_runners, // task runners
nullptr, // window
{}, // snapshot delegate
{}, // IO Manager
DART_VM_SERVICE_ISOLATE_NAME, // script uri
DART_VM_SERVICE_ISOLATE_NAME, // script entrypoint
flags // flags
vm_data->GetSettings(), // settings
vm_data->GetIsolateSnapshot(), // isolate snapshot
vm_data->GetSharedSnapshot(), // shared snapshot
null_task_runners, // task runners
nullptr, // window
{}, // snapshot delegate
{}, // IO Manager
DART_VM_SERVICE_ISOLATE_NAME, // script uri
DART_VM_SERVICE_ISOLATE_NAME, // script entrypoint
flags // flags
);

std::shared_ptr<DartIsolate> service_isolate = weak_service_isolate.lock();
Expand All @@ -556,7 +552,13 @@ Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate(
return nullptr;
}

vm->GetServiceProtocol().ToggleHooks(true);
if (auto service_protocol = DartVMRef::GetServiceProtocol()) {
service_protocol->ToggleHooks(true);
} else {
FML_DLOG(ERROR)
<< "Could not acquire the service protocol handlers. This might be "
"because the VM has already begun teardown on another thread.";
}

return service_isolate->isolate();
}
Expand Down Expand Up @@ -612,16 +614,13 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair(
std::unique_ptr<std::shared_ptr<DartIsolate>> embedder_isolate(
p_parent_embedder_isolate);

if (embedder_isolate == nullptr ||
(*embedder_isolate)->GetDartVM() == nullptr) {
if (embedder_isolate == nullptr) {
*error =
strdup("Parent isolate did not have embedder specific callback data.");
FML_DLOG(ERROR) << *error;
return {nullptr, {}};
}

DartVM* const vm = (*embedder_isolate)->GetDartVM();

if (!is_root_isolate) {
auto* raw_embedder_isolate = embedder_isolate.release();

Expand All @@ -630,7 +629,7 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair(

embedder_isolate = std::make_unique<std::shared_ptr<DartIsolate>>(
std::make_shared<DartIsolate>(
vm, // vm
(*raw_embedder_isolate)->GetSettings(), // settings
(*raw_embedder_isolate)->GetIsolateSnapshot(), // isolate_snapshot
(*raw_embedder_isolate)->GetSharedSnapshot(), // shared_snapshot
null_task_runners, // task_runners
Expand Down Expand Up @@ -703,11 +702,11 @@ void DartIsolate::DartIsolateCleanupCallback(
delete embedder_isolate;
}

fml::RefPtr<DartSnapshot> DartIsolate::GetIsolateSnapshot() const {
fml::RefPtr<const DartSnapshot> DartIsolate::GetIsolateSnapshot() const {
return isolate_snapshot_;
}

fml::RefPtr<DartSnapshot> DartIsolate::GetSharedSnapshot() const {
fml::RefPtr<const DartSnapshot> DartIsolate::GetSharedSnapshot() const {
return shared_snapshot_;
}

Expand Down
26 changes: 13 additions & 13 deletions runtime/dart_isolate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ class DartIsolate : public UIDartState {
// bindings. From the VM's perspective, this isolate is not special in any
// way.
static std::weak_ptr<DartIsolate> CreateRootIsolate(
DartVM* vm,
fml::RefPtr<DartSnapshot> isolate_snapshot,
fml::RefPtr<DartSnapshot> shared_snapshot,
const Settings& settings,
fml::RefPtr<const DartSnapshot> isolate_snapshot,
fml::RefPtr<const DartSnapshot> shared_snapshot,
TaskRunners task_runners,
std::unique_ptr<Window> window,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
Expand All @@ -52,9 +52,9 @@ class DartIsolate : public UIDartState {
std::string advisory_script_entrypoint,
Dart_IsolateFlags* flags = nullptr);

DartIsolate(DartVM* vm,
fml::RefPtr<DartSnapshot> isolate_snapshot,
fml::RefPtr<DartSnapshot> shared_snapshot,
DartIsolate(const Settings& settings,
fml::RefPtr<const DartSnapshot> isolate_snapshot,
fml::RefPtr<const DartSnapshot> shared_snapshot,
TaskRunners task_runners,
fml::WeakPtr<SnapshotDelegate> snapshot_delegate,
fml::WeakPtr<IOManager> io_manager,
Expand All @@ -64,6 +64,8 @@ class DartIsolate : public UIDartState {

~DartIsolate() override;

const Settings& GetSettings() const;

Phase GetPhase() const;

FML_WARN_UNUSED_RESULT
Expand All @@ -86,11 +88,9 @@ class DartIsolate : public UIDartState {

void AddIsolateShutdownCallback(fml::closure closure);

DartVM* GetDartVM() const;

fml::RefPtr<DartSnapshot> GetIsolateSnapshot() const;
fml::RefPtr<const DartSnapshot> GetIsolateSnapshot() const;

fml::RefPtr<DartSnapshot> GetSharedSnapshot() const;
fml::RefPtr<const DartSnapshot> GetSharedSnapshot() const;

std::weak_ptr<DartIsolate> GetWeakIsolatePtr();

Expand All @@ -109,10 +109,10 @@ class DartIsolate : public UIDartState {
};
friend class DartVM;

DartVM* const vm_ = nullptr;
Phase phase_ = Phase::Unknown;
const fml::RefPtr<DartSnapshot> isolate_snapshot_;
const fml::RefPtr<DartSnapshot> shared_snapshot_;
const Settings settings_;
const fml::RefPtr<const DartSnapshot> isolate_snapshot_;
const fml::RefPtr<const DartSnapshot> shared_snapshot_;
std::vector<std::shared_ptr<const fml::Mapping>> kernel_buffers_;
std::vector<std::unique_ptr<AutoFireClosure>> shutdown_callbacks_;
ChildIsolatePreparer child_isolate_preparer_ = nullptr;
Expand Down
Loading

0 comments on commit c991647

Please sign in to comment.