From bbd17510db39b82f67cc19272ad3dd7cc348556f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 9 Dec 2022 14:36:36 +0100 Subject: [PATCH 1/3] src: use CreateEnvironment instead of inlining its code where possible We had a number of places in which we created an `Environment` instance by performing each step in `CreateEnvironment` manually. Instead, just call the function itself. --- src/api/environment.cc | 26 ++++++++++++++++++-------- src/node.h | 5 ++++- src/node_main_instance.cc | 15 ++------------- src/node_snapshotable.cc | 27 ++++++++++++++------------- 4 files changed, 38 insertions(+), 35 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 0cbbe3fb048517..1f577f0b0c3a32 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -375,23 +375,33 @@ Environment* CreateEnvironment( // options than the global parse call. Environment* env = new Environment( isolate_data, context, args, exec_args, nullptr, flags, thread_id); + + auto initialize_inspector = [&]() { #if HAVE_INSPECTOR - if (env->should_create_inspector()) { - if (inspector_parent_handle) { - env->InitializeInspector( - std::move(static_cast( - inspector_parent_handle.get())->impl)); - } else { - env->InitializeInspector({}); + if (env->should_create_inspector()) { + if (inspector_parent_handle) { + env->InitializeInspector( + std::move(static_cast( + inspector_parent_handle.get()) + ->impl)); + } else { + env->InitializeInspector({}); + } } - } #endif + }; + + if (!(flags & EnvironmentFlags::kInspectorOnlyAfterBootstrap)) + initialize_inspector(); if (env->principal_realm()->RunBootstrapping().IsEmpty()) { FreeEnvironment(env); return nullptr; } + if (flags & EnvironmentFlags::kInspectorOnlyAfterBootstrap) + initialize_inspector(); + return env; } diff --git a/src/node.h b/src/node.h index 561af155561923..712f668a83688d 100644 --- a/src/node.h +++ b/src/node.h @@ -560,7 +560,10 @@ enum Flags : uint64_t { // This control is needed by embedders who may not want to initialize the V8 // inspector in situations where one has already been created, // e.g. Blink's in Chromium. - kNoCreateInspector = 1 << 9 + kNoCreateInspector = 1 << 9, + // Whether to enable the V8 inspector during Node.js bootstrap or not. + // This has no effect if kNoCreateInspector is set. + kInspectorOnlyAfterBootstrap = 1 << 10 }; } // namespace EnvironmentFlags diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 3e0167fdddea12..5060c69499f813 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -191,19 +191,8 @@ NodeMainInstance::CreateMainEnvironment(ExitCode* exit_code) { context = NewContext(isolate_); CHECK(!context.IsEmpty()); Context::Scope context_scope(context); - env.reset(new Environment(isolate_data_.get(), - context, - args_, - exec_args_, - nullptr, - EnvironmentFlags::kDefaultFlags, - {})); -#if HAVE_INSPECTOR - env->InitializeInspector({}); -#endif - if (env->principal_realm()->RunBootstrapping().IsEmpty()) { - return nullptr; - } + env.reset( + CreateEnvironment(isolate_data_.get(), context, args_, exec_args_)); } return env; diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 145e9fa9c18c3c..9b9e66d9ca4073 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1151,16 +1151,20 @@ ExitCode SnapshotBuilder::Generate(SnapshotData* out, Context::Scope context_scope(main_context); // Create the environment. - env = new Environment(main_instance->isolate_data(), - main_context, - args, - exec_args, - nullptr, - node::EnvironmentFlags::kDefaultFlags, - {}); - - // Run scripts in lib/internal/bootstrap/ - if (env->principal_realm()->RunBootstrapping().IsEmpty()) { + uint64_t env_flags = EnvironmentFlags::kDefaultFlags | + EnvironmentFlags::kInspectorOnlyAfterBootstrap; + if (snapshot_type != SnapshotMetadata::Type::kFullyCustomized) { + env_flags |= EnvironmentFlags::kNoCreateInspector; + } + + env = CreateEnvironment(main_instance->isolate_data(), + main_context, + args, + exec_args, + static_cast(env_flags)); + + // This already ran scripts in lib/internal/bootstrap/, if it fails return + if (env == nullptr) { return ExitCode::kBootstrapFailure; } // If --build-snapshot is true, lib/internal/main/mksnapshot.js would be @@ -1169,9 +1173,6 @@ ExitCode SnapshotBuilder::Generate(SnapshotData* out, // could also explore snapshotting other kinds of execution modes // in the future). if (snapshot_type == SnapshotMetadata::Type::kFullyCustomized) { -#if HAVE_INSPECTOR - env->InitializeInspector({}); -#endif if (LoadEnvironment(env, StartExecutionCallback{}).IsEmpty()) { return ExitCode::kGenericUserError; } From 6368ff01282a304bc3c0ea9457ca7adeed02a412 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 16 Dec 2022 19:23:54 +0100 Subject: [PATCH 2/3] fixup! src: use CreateEnvironment instead of inlining its code where possible --- src/api/environment.cc | 25 ++++++++----------------- src/node.h | 5 +---- src/node_snapshotable.cc | 8 ++++---- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 1f577f0b0c3a32..529929deb63524 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -376,32 +376,23 @@ Environment* CreateEnvironment( Environment* env = new Environment( isolate_data, context, args, exec_args, nullptr, flags, thread_id); - auto initialize_inspector = [&]() { #if HAVE_INSPECTOR - if (env->should_create_inspector()) { - if (inspector_parent_handle) { - env->InitializeInspector( - std::move(static_cast( - inspector_parent_handle.get()) - ->impl)); - } else { - env->InitializeInspector({}); - } + if (env->should_create_inspector()) { + if (inspector_parent_handle) { + env->InitializeInspector(std::move( + static_cast(inspector_parent_handle.get()) + ->impl)); + } else { + env->InitializeInspector({}); } + } #endif - }; - - if (!(flags & EnvironmentFlags::kInspectorOnlyAfterBootstrap)) - initialize_inspector(); if (env->principal_realm()->RunBootstrapping().IsEmpty()) { FreeEnvironment(env); return nullptr; } - if (flags & EnvironmentFlags::kInspectorOnlyAfterBootstrap) - initialize_inspector(); - return env; } diff --git a/src/node.h b/src/node.h index 712f668a83688d..561af155561923 100644 --- a/src/node.h +++ b/src/node.h @@ -560,10 +560,7 @@ enum Flags : uint64_t { // This control is needed by embedders who may not want to initialize the V8 // inspector in situations where one has already been created, // e.g. Blink's in Chromium. - kNoCreateInspector = 1 << 9, - // Whether to enable the V8 inspector during Node.js bootstrap or not. - // This has no effect if kNoCreateInspector is set. - kInspectorOnlyAfterBootstrap = 1 << 10 + kNoCreateInspector = 1 << 9 }; } // namespace EnvironmentFlags diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 9b9e66d9ca4073..3b2617e7addc7e 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1152,10 +1152,7 @@ ExitCode SnapshotBuilder::Generate(SnapshotData* out, // Create the environment. uint64_t env_flags = EnvironmentFlags::kDefaultFlags | - EnvironmentFlags::kInspectorOnlyAfterBootstrap; - if (snapshot_type != SnapshotMetadata::Type::kFullyCustomized) { - env_flags |= EnvironmentFlags::kNoCreateInspector; - } + EnvironmentFlags::kNoCreateInspector; env = CreateEnvironment(main_instance->isolate_data(), main_context, @@ -1173,6 +1170,9 @@ ExitCode SnapshotBuilder::Generate(SnapshotData* out, // could also explore snapshotting other kinds of execution modes // in the future). if (snapshot_type == SnapshotMetadata::Type::kFullyCustomized) { +#if HAVE_INSPECTOR + env->InitializeInspector({}); +#endif if (LoadEnvironment(env, StartExecutionCallback{}).IsEmpty()) { return ExitCode::kGenericUserError; } From b634c382bbf6060273d95ccb96b1804211fbe8e6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 16 Dec 2022 19:25:09 +0100 Subject: [PATCH 3/3] fixup! fixup! src: use CreateEnvironment instead of inlining its code where possible --- src/node_snapshotable.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 3b2617e7addc7e..92dc2c165e4471 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1151,6 +1151,10 @@ ExitCode SnapshotBuilder::Generate(SnapshotData* out, Context::Scope context_scope(main_context); // Create the environment. + // It's not guaranteed that a context that goes through + // v8_inspector::V8Inspector::contextCreated() is runtime-independent, + // so do not start the inspector on the main context when building + // the default snapshot. uint64_t env_flags = EnvironmentFlags::kDefaultFlags | EnvironmentFlags::kNoCreateInspector;