src: initialize inspector before RunBootstrapping()

This is necessary for `--inspect-brk-node` to work, and for the
inspector to be aware of scripts created before bootstrapping.

Fixes: https://github.com/nodejs/node/issues/32648
Refs: https://github.com/nodejs/node/pull/30467#discussion_r396879908

PR-URL: https://github.com/nodejs/node/pull/32672
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

src/api/environment.cc

@@ -330,6 +330,19 @@ void FreeIsolateData(IsolateData* isolate_data) {
   delete isolate_data;
 }
 
+InspectorParentHandle::~InspectorParentHandle() {}
+
+// Hide the internal handle class from the public API.
+#if HAVE_INSPECTOR
+struct InspectorParentHandleImpl : public InspectorParentHandle {
+  std::unique_ptr<inspector::ParentInspectorHandle> impl;
+
+  explicit InspectorParentHandleImpl(
+      std::unique_ptr<inspector::ParentInspectorHandle>&& impl)
+    : impl(std::move(impl)) {}
+};
+#endif
+
 Environment* CreateEnvironment(IsolateData* isolate_data,
                                Local<Context> context,
                                int argc,
@@ -348,7 +361,8 @@ Environment* CreateEnvironment(
     const std::vector<std::string>& args,
     const std::vector<std::string>& exec_args,
     EnvironmentFlags::Flags flags,
-    ThreadId thread_id) {
+    ThreadId thread_id,
+    std::unique_ptr<InspectorParentHandle> inspector_parent_handle) {
   Isolate* isolate = context->GetIsolate();
   HandleScope handle_scope(isolate);
   Context::Scope context_scope(context);
@@ -365,6 +379,16 @@ Environment* CreateEnvironment(
     env->set_abort_on_uncaught_exception(false);
   }
 
+#if HAVE_INSPECTOR
+  if (inspector_parent_handle) {
+    env->InitializeInspector(
+        std::move(static_cast<InspectorParentHandleImpl*>(
+            inspector_parent_handle.get())->impl));
+  } else {
+    env->InitializeInspector({});
+  }
+#endif
+
   if (env->RunBootstrapping().IsEmpty()) {
     FreeEnvironment(env);
     return nullptr;
@@ -394,19 +418,6 @@ void FreeEnvironment(Environment* env) {
   delete env;
 }
 
-InspectorParentHandle::~InspectorParentHandle() {}
-
-// Hide the internal handle class from the public API.
-#if HAVE_INSPECTOR
-struct InspectorParentHandleImpl : public InspectorParentHandle {
-  std::unique_ptr<inspector::ParentInspectorHandle> impl;
-
-  explicit InspectorParentHandleImpl(
-      std::unique_ptr<inspector::ParentInspectorHandle>&& impl)
-    : impl(std::move(impl)) {}
-};
-#endif
-
 NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
     Environment* env,
     ThreadId thread_id,
@@ -430,27 +441,17 @@ void LoadEnvironment(Environment* env) {
 MaybeLocal<Value> LoadEnvironment(
     Environment* env,
     StartExecutionCallback cb,
-    std::unique_ptr<InspectorParentHandle> inspector_parent_handle) {
+    std::unique_ptr<InspectorParentHandle> removeme) {
   env->InitializeLibuv(per_process::v8_is_profiling);
   env->InitializeDiagnostics();
 
-#if HAVE_INSPECTOR
-  if (inspector_parent_handle) {
-    env->InitializeInspector(
-        std::move(static_cast<InspectorParentHandleImpl*>(
-            inspector_parent_handle.get())->impl));
-  } else {
-    env->InitializeInspector({});
-  }
-#endif
-
   return StartExecution(env, cb);
 }
 
 MaybeLocal<Value> LoadEnvironment(
     Environment* env,
     const char* main_script_source_utf8,
-    std::unique_ptr<InspectorParentHandle> inspector_parent_handle) {
+    std::unique_ptr<InspectorParentHandle> removeme) {
   CHECK_NOT_NULL(main_script_source_utf8);
   return LoadEnvironment(
       env,
@@ -475,8 +476,7 @@ MaybeLocal<Value> LoadEnvironment(
             env->process_object(),
             env->native_module_require()};
         return ExecuteBootstrapper(env, name.c_str(), &params, &args);
-      },
-      std::move(inspector_parent_handle));
+      });
 }
 
 Environment* GetCurrentEnvironment(Local<Context> context) {

src/node.h

@@ -420,6 +420,10 @@ enum Flags : uint64_t {
 };
 }  // namespace EnvironmentFlags
 
+struct InspectorParentHandle {
+  virtual ~InspectorParentHandle();
+};
+
 // TODO(addaleax): Maybe move per-Environment options parsing here.
 // Returns nullptr when the Environment cannot be created e.g. there are
 // pending JavaScript exceptions.
@@ -436,16 +440,14 @@ NODE_EXTERN Environment* CreateEnvironment(
     const std::vector<std::string>& args,
     const std::vector<std::string>& exec_args,
     EnvironmentFlags::Flags flags = EnvironmentFlags::kDefaultFlags,
-    ThreadId thread_id = {} /* allocates a thread id automatically */);
+    ThreadId thread_id = {} /* allocates a thread id automatically */,
+    std::unique_ptr<InspectorParentHandle> inspector_parent_handle = {});
 
-struct InspectorParentHandle {
-  virtual ~InspectorParentHandle();
-};
 // Returns a handle that can be passed to `LoadEnvironment()`, making the
 // child Environment accessible to the inspector as if it were a Node.js Worker.
 // `child_thread_id` can be created using `AllocateEnvironmentThreadId()`
 // and then later passed on to `CreateEnvironment()` to create the child
-// Environment.
+// Environment, together with the inspector handle.
 // This method should not be called while the parent Environment is active
 // on another thread.
 NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
@@ -463,14 +465,16 @@ using StartExecutionCallback =
 
 // TODO(addaleax): Deprecate this in favour of the MaybeLocal<> overload.
 NODE_EXTERN void LoadEnvironment(Environment* env);
+// The `InspectorParentHandle` arguments here are ignored and not used.
+// For passing `InspectorParentHandle`, use `CreateEnvironment()`.
 NODE_EXTERN v8::MaybeLocal<v8::Value> LoadEnvironment(
     Environment* env,
     StartExecutionCallback cb,
-    std::unique_ptr<InspectorParentHandle> inspector_parent_handle = {});
+    std::unique_ptr<InspectorParentHandle> ignored_donotuse_removeme = {});
 NODE_EXTERN v8::MaybeLocal<v8::Value> LoadEnvironment(
     Environment* env,
     const char* main_script_source_utf8,
-    std::unique_ptr<InspectorParentHandle> inspector_parent_handle = {});
+    std::unique_ptr<InspectorParentHandle> ignored_donotuse_removeme = {});
 NODE_EXTERN void FreeEnvironment(Environment* env);
 
 // Set a callback that is called when process.exit() is called from JS,

src/node_worker.cc

@@ -310,7 +310,8 @@ void Worker::Run() {
             std::move(argv_),
             std::move(exec_argv_),
             EnvironmentFlags::kNoFlags,
-            thread_id_));
+            thread_id_,
+            std::move(inspector_parent_handle_)));
         if (is_stopped()) return;
         CHECK_NOT_NULL(env_);
         env_->set_env_vars(std::move(env_vars_));
@@ -328,12 +329,8 @@ void Worker::Run() {
       {
         CreateEnvMessagePort(env_.get());
         Debug(this, "Created message port for worker %llu", thread_id_.id);
-        if (LoadEnvironment(env_.get(),
-                            StartExecutionCallback{},
-                            std::move(inspector_parent_handle_))
-                .IsEmpty()) {
+        if (LoadEnvironment(env_.get(), StartExecutionCallback{}).IsEmpty())
           return;
-        }
 
         Debug(this, "Loaded environment for worker %llu", thread_id_.id);
       }

test/cctest/node_test_fixture.h

@@ -135,7 +135,10 @@ class EnvironmentTestFixture : public NodeTestFixture {
  public:
   class Env {
    public:
-    Env(const v8::HandleScope& handle_scope, const Argv& argv) {
+    Env(const v8::HandleScope& handle_scope,
+        const Argv& argv,
+        node::EnvironmentFlags::Flags flags =
+            node::EnvironmentFlags::kDefaultFlags) {
       auto isolate = handle_scope.GetIsolate();
       context_ = node::NewContext(isolate);
       CHECK(!context_.IsEmpty());
@@ -145,10 +148,13 @@ class EnvironmentTestFixture : public NodeTestFixture {
                                               &NodeTestFixture::current_loop,
                                               platform.get());
       CHECK_NE(nullptr, isolate_data_);
+      std::vector<std::string> args(*argv, *argv + 1);
+      std::vector<std::string> exec_args(*argv, *argv + 1);
       environment_ = node::CreateEnvironment(isolate_data_,
                                              context_,
-                                             1, *argv,
-                                             argv.nr_args(), *argv);
+                                             args,
+                                             exec_args,
+                                             flags);
       CHECK_NE(nullptr, environment_);
     }
 

test/cctest/test_environment.cc

@@ -168,8 +168,9 @@ TEST_F(EnvironmentTest, AtExitRunsJS) {
 TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) {
   const v8::HandleScope handle_scope(isolate_);
   const Argv argv;
+  // Only one of the Environments can have default flags and own the inspector.
   Env env1 {handle_scope, argv};
-  Env env2 {handle_scope, argv};
+  Env env2 {handle_scope, argv, node::EnvironmentFlags::kNoFlags};
 
   AtExit(*env1, at_exit_callback1);
   AtExit(*env2, at_exit_callback2);
@@ -334,7 +335,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) {
       "        id: 1,\n"
       "        method: 'Runtime.evaluate',\n"
       "        params: {\n"
-      "          expression: 'global.variableFromParent = 42;'\n"
+      "          expression: 'globalThis.variableFromParent = 42;'\n"
       "        }\n"
       "      })\n"
       "    });\n"
@@ -401,14 +402,14 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) {
           { "dummy" },
           {},
           node::EnvironmentFlags::kNoFlags,
-          data->thread_id);
+          data->thread_id,
+          std::move(data->inspector_parent_handle));
       CHECK_NOT_NULL(environment);
 
       v8::Local<v8::Value> extracted_value = LoadEnvironment(
           environment,
           "while (!global.variableFromParent) {}\n"
-          "return global.variableFromParent;",
-          std::move(data->inspector_parent_handle)).ToLocalChecked();
+          "return global.variableFromParent;").ToLocalChecked();
 
       uv_run(&loop, UV_RUN_DEFAULT);
       CHECK(extracted_value->IsInt32());

test/parallel/test-inspector-inspect-brk-node.js

@@ -0,0 +1,28 @@
+'use strict';
+const common = require('../common');
+
+// Regression test for https://github.com/nodejs/node/issues/32648
+
+common.skipIfInspectorDisabled();
+
+const { NodeInstance } = require('../common/inspector-helper.js');
+
+async function runTest() {
+  const child = new NodeInstance(['--inspect-brk-node=0', '-p', '42']);
+  const session = await child.connectInspectorSession();
+  await session.send({ method: 'Runtime.enable' });
+  await session.send({ method: 'Debugger.enable' });
+  await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
+  await session.waitForNotification((notification) => {
+    // The main assertion here is that we do hit the loader script first.
+    return notification.method === 'Debugger.scriptParsed' &&
+           notification.params.url === 'internal/bootstrap/loaders.js';
+  });
+
+  await session.waitForNotification('Debugger.paused');
+  await session.send({ method: 'Debugger.resume' });
+  await session.waitForNotification('Debugger.paused');
+  await session.runToCompletion();
+}
+
+runTest().then(common.mustCall());