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: #32648
Refs: #30467 (comment)

Author: addaleax

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,

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());