Skip to content

Commit

Permalink
Ensure root isolate create callback is invoked before the isolate is …
Browse files Browse the repository at this point in the history
…in the running phase. (flutter#22041)

Embedders that have access to the Dart native API (only Fuchsia now) may perform
library setup in the isolate create callback. The engine used to depend on the
fact the root isolate entrypoint is invoked in the next iteration of message
loop (via the `_startIsolate` trampoline in `isolate_patch.dart`) to ensure that
library setup occur before the main entrypoint was invoked. However, due to
differences in the way in which message loops are setup in Fuchsia, this
entrypoint was run before the callback could be executed. Dart code on Fuchsia
also has the ability to access the underlying event loops directly. This patch
moves the invocation of the create callback to before user dart code has a
chance to run. This difference in behavior on Fuchsia became an issue when the
isolate initialization was reworked in flutter#21820
for null-safety.

Another issue was discovered in that the callback was being invoked twice, I
fixed that too and added a test.

Fixes flutter/flutter#68732
  • Loading branch information
chinmaygarde authored Oct 21, 2020
1 parent fc72bd2 commit f459a86
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 26 deletions.
6 changes: 5 additions & 1 deletion common/settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ using MappingsCallback = std::function<Mappings(void)>;

using FrameRasterizedCallback = std::function<void(const FrameTiming&)>;

class DartIsolate;

struct Settings {
Settings();

Expand Down Expand Up @@ -169,7 +171,9 @@ struct Settings {
TaskObserverRemove task_observer_remove;
// The main isolate is current when this callback is made. This is a good spot
// to perform native Dart bindings for libraries not built in.
fml::closure root_isolate_create_callback;
std::function<void(const DartIsolate&)> root_isolate_create_callback;
// TODO(68738): Update isolate callbacks in settings to accept an additional
// DartIsolate parameter.
fml::closure isolate_create_callback;
// The isolate is not current and may have already been destroyed when this
// call is made.
Expand Down
26 changes: 11 additions & 15 deletions runtime/dart_isolate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -148,21 +148,21 @@ std::weak_ptr<DartIsolate> DartIsolate::CreateRunningRootIsolate(
return {};
}

if (!isolate->RunFromLibrary(dart_entrypoint_library, //
dart_entrypoint, //
settings.dart_entrypoint_args, //
settings.root_isolate_create_callback //
if (settings.root_isolate_create_callback) {
// Isolate callbacks always occur in isolate scope and before user code has
// had a chance to run.
tonic::DartState::Scope scope(isolate.get());
settings.root_isolate_create_callback(*isolate.get());
}

if (!isolate->RunFromLibrary(dart_entrypoint_library, //
dart_entrypoint, //
settings.dart_entrypoint_args //
)) {
FML_LOG(ERROR) << "Could not run the run main Dart entrypoint.";
return {};
}

if (settings.root_isolate_create_callback) {
// Isolate callbacks always occur in isolate scope.
tonic::DartState::Scope scope(isolate.get());
settings.root_isolate_create_callback();
}

if (settings.root_isolate_shutdown_callback) {
isolate->AddIsolateShutdownCallback(
settings.root_isolate_shutdown_callback);
Expand Down Expand Up @@ -617,8 +617,7 @@ bool DartIsolate::MarkIsolateRunnable() {

bool DartIsolate::RunFromLibrary(std::optional<std::string> library_name,
std::optional<std::string> entrypoint,
const std::vector<std::string>& args,
const fml::closure& on_run) {
const std::vector<std::string>& args) {
TRACE_EVENT0("flutter", "DartIsolate::RunFromLibrary");
if (phase_ != Phase::Ready) {
return false;
Expand All @@ -644,9 +643,6 @@ bool DartIsolate::RunFromLibrary(std::optional<std::string> library_name,

phase_ = Phase::Running;

if (on_run) {
on_run();
}
return true;
}

Expand Down
8 changes: 1 addition & 7 deletions runtime/dart_isolate.h
Original file line number Diff line number Diff line change
Expand Up @@ -342,20 +342,14 @@ class DartIsolate : public UIDartState {
/// supplied entrypoint.
/// @param[in] entrypoint The entrypoint in `library_name`
/// @param[in] args A list of string arguments to the entrypoint.
/// @param[in] on_run A callback to run in isolate scope after the
/// main entrypoint has been invoked. There is no
/// isolate scope current on the thread once this
/// method returns.
///
/// @return If the isolate successfully transitioned to the running phase
/// and the main entrypoint was invoked.
///
[[nodiscard]] bool RunFromLibrary(std::optional<std::string> library_name,
std::optional<std::string> entrypoint,
const std::vector<std::string>& args,
const fml::closure& on_run = nullptr);
const std::vector<std::string>& args);

public:
//----------------------------------------------------------------------------
/// @brief Transition the isolate to the `Phase::Shutdown` phase. The
/// only thing left to do is to collect the isolate.
Expand Down
27 changes: 27 additions & 0 deletions runtime/dart_isolate_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -375,5 +375,32 @@ TEST_F(DartIsolateTest, CanCreateServiceIsolate) {
ASSERT_TRUE(root_isolate->Shutdown());
}

TEST_F(DartIsolateTest,
RootIsolateCreateCallbackIsMadeOnceAndBeforeIsolateRunning) {
ASSERT_FALSE(DartVMRef::IsInstanceRunning());
auto settings = CreateSettingsForFixture();
size_t create_callback_count = 0u;
settings.root_isolate_create_callback =
[&create_callback_count](const auto& isolate) {
ASSERT_EQ(isolate.GetPhase(), DartIsolate::Phase::Ready);
create_callback_count++;
ASSERT_NE(::Dart_CurrentIsolate(), nullptr);
};
auto vm_ref = DartVMRef::Create(settings);
TaskRunners task_runners(GetCurrentTestName(), //
GetCurrentTaskRunner(), //
GetCurrentTaskRunner(), //
GetCurrentTaskRunner(), //
GetCurrentTaskRunner() //
);
{
auto isolate = RunDartCodeInIsolate(vm_ref, settings, task_runners, "main",
{}, GetFixturesPath());
ASSERT_TRUE(isolate);
ASSERT_EQ(isolate->get()->GetPhase(), DartIsolate::Phase::Running);
}
ASSERT_EQ(create_callback_count, 1u);
}

} // namespace testing
} // namespace flutter
5 changes: 2 additions & 3 deletions shell/platform/embedder/embedder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -841,9 +841,8 @@ FlutterEngineResult FlutterEngineInitialize(size_t version,
if (SAFE_ACCESS(args, root_isolate_create_callback, nullptr) != nullptr) {
VoidCallback callback =
SAFE_ACCESS(args, root_isolate_create_callback, nullptr);
settings.root_isolate_create_callback = [callback, user_data]() {
callback(user_data);
};
settings.root_isolate_create_callback =
[callback, user_data](const auto& isolate) { callback(user_data); };
}

flutter::PlatformViewEmbedder::UpdateSemanticsNodesCallback
Expand Down

0 comments on commit f459a86

Please sign in to comment.