From 47469cf202b981facf9fc4abec8eccbfd817504e Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 20 Oct 2020 13:57:45 -0700 Subject: [PATCH 1/2] Determine null-safety isolate flags for launches of the service isolate. Previously, there was as assumption was that the service isolate launch flags would be inherited from the parent isolate (the root isolate). This does not seem to be the case the isolate launch flags must specify the null-safety characteristics. Since the application kernel for the service isolate is not available, it is unclear of the VM is able to determine the same from the isolate snapshot. The added test seems to indicate that it does. However, it would be good to get a clarification from the VM team as to weather this usage is correct. Fixes https://github.com/flutter/flutter/issues/68561 --- common/settings.h | 8 +++++ runtime/dart_isolate.cc | 6 ++++ runtime/dart_isolate_unittests.cc | 54 +++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/common/settings.h b/common/settings.h index 17408322eedb3..8de173511d4c4 100644 --- a/common/settings.h +++ b/common/settings.h @@ -175,6 +175,14 @@ struct Settings { // call is made. fml::closure root_isolate_shutdown_callback; fml::closure isolate_shutdown_callback; + // A callback made in the isolate scope of the service isolate when it is + // launched. Care must be taken to ensure that callers are assigning callbacks + // to the settings object used to launch the VM. If an existing VM is used to + // launch an isolate using these settings, the callback will be ignored as the + // service isolate has already been launched. Also, this callback will only be + // made in the modes in which the service isolate is eligible for launch + // (debug and profile). + fml::closure service_isolate_create_callback; // The callback made on the UI thread in an isolate scope when the engine // detects that the framework is idle. The VM also uses this time to perform // tasks suitable when idling. Due to this, embedders are still advised to be diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 2fa50132e6750..bf34b1ae5a016 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -697,6 +697,8 @@ Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate( nullptr, nullptr, nullptr, nullptr); flags->load_vmservice_library = true; + flags->null_safety = + vm_data->GetIsolateSnapshot()->IsNullSafetyEnabled(nullptr); std::weak_ptr weak_service_isolate = DartIsolate::CreateRootIsolate( @@ -739,6 +741,10 @@ Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate( return nullptr; } + if (auto callback = vm_data->GetSettings().service_isolate_create_callback) { + callback(); + } + if (auto service_protocol = DartVMRef::GetServiceProtocol()) { service_protocol->ToggleHooks(true); } else { diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index c95235025ee8e..bfdbfa9914669 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -321,5 +321,59 @@ TEST_F(DartIsolateTest, CanRecieveArguments) { Wait(); } +TEST_F(DartIsolateTest, CanCreateServiceIsolate) { +#if (FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_DEBUG) && \ + (FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_PROFILE) + GTEST_SKIP(); +#endif + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + fml::AutoResetWaitableEvent service_isolate_latch; + auto settings = CreateSettingsForFixture(); + settings.enable_observatory = true; + settings.observatory_port = 0; + settings.observatory_host = "127.0.0.1"; + settings.enable_service_port_fallback = true; + settings.service_isolate_create_callback = [&service_isolate_latch]() { + service_isolate_latch.Signal(); + }; + auto vm_ref = DartVMRef::Create(settings); + ASSERT_TRUE(vm_ref); + auto vm_data = vm_ref.GetVMData(); + ASSERT_TRUE(vm_data); + TaskRunners task_runners(GetCurrentTestName(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner() // + ); + + auto isolate_configuration = + IsolateConfiguration::InferFromSettings(settings); + auto weak_isolate = DartIsolate::CreateRunningRootIsolate( + vm_data->GetSettings(), // settings + vm_data->GetIsolateSnapshot(), // isolate snapshot + std::move(task_runners), // task runners + nullptr, // window + {}, // snapshot delegate + {}, // hint freed delegate + {}, // io manager + {}, // unref queue + {}, // image decoder + "main.dart", // advisory uri + "main", // advisory entrypoint, + DartIsolate::Flags{}, // flags + settings.isolate_create_callback, // isolate create callback + settings.isolate_shutdown_callback, // isolate shutdown callback + "main", // dart entrypoint + std::nullopt, // dart entrypoint library + std::move(isolate_configuration) // isolate configuration + ); + auto root_isolate = weak_isolate.lock(); + ASSERT_TRUE(root_isolate); + ASSERT_EQ(root_isolate->GetPhase(), DartIsolate::Phase::Running); + service_isolate_latch.Wait(); + ASSERT_TRUE(root_isolate->Shutdown()); +} + } // namespace testing } // namespace flutter From 370bbd3c8e980920c7d4dd92100873bfb1c9dd98 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 20 Oct 2020 15:30:37 -0700 Subject: [PATCH 2/2] Address Siva's PR comments. --- runtime/dart_isolate.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index bf34b1ae5a016..ae655f00e07cf 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -697,8 +697,14 @@ Dart_Isolate DartIsolate::DartCreateAndStartServiceIsolate( nullptr, nullptr, nullptr, nullptr); flags->load_vmservice_library = true; + +#if (FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_DEBUG) + // TODO(68663): The service isolate in debug mode is always launched without + // sound null safety. Fix after the isolate snapshot data is created with the + // right flags. flags->null_safety = vm_data->GetIsolateSnapshot()->IsNullSafetyEnabled(nullptr); +#endif std::weak_ptr weak_service_isolate = DartIsolate::CreateRootIsolate(