src: allow preventing InitializeInspector in env

[codebytere]

This PR adds a new EnvironmentFlags option: kNoInitializeInspector.

8c5ad13 made it such that env->InitializeInspector was called in CreateEnvironment
no matter what, which creates an issue for embedders seeking to manage the InspectorAgent themselves as Electron does. This new flag mitigates that issue by allowing embedders to prevent that invocation.

cc @joyeecheung

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[addaleax]

I think this needs to come with a test, in particular one that checks that using the inspector module does not crash when it’s being used while this flag is set

It would also be nice to have some documentation on how this differs from not specifying kOwnsInspector? What problems is Electron having here?

[codebytere]

@joyeecheung @addaleax sorry for the delay on this! So there are quite a few things going on - this is definitely not the most robust solution and i'm very open to others.

At a high level - Electron wants to control the inspector agent. We do this here for timing reasons and also so that we can extend it a bit. As such, we want to start it ourselves, which conflicts with the call to env->InitializeInspector added in 8c5ad13. Previously, it was in CreateMainEnvironment, which we never called, so this wasn’t an issue. 

Unfortunately, we need to have an env to create our debugger as seen in the above linked class, but per #30467 (comment) this causes --inspect-brk-node not to work (it’s been broken, we already knew about it).

Additionally, the inspector agent isn’t exposed at present - Electron does so ourselves. 

Ultimately, ideally, we’re looking for a way to control the inspector initialization process ourselves while also allowing --inspect-brk-node to work (though the latter is lower priority).

[addaleax]

@codebytere Can you expand more on the timing/extension reasons that make Electron want to start the Agent itself? It sounds like that is the root of the issue here.

I would probably not move forward with this PR in its current form, because I don’t think it’s a good choice for Node.js to add a public API feature where the intended use entails that private APIs always end up being used in conjunction with it. In that case, it’s probably better to find out what the right API surface here is.

(Also, to avoid confusion: When you say you want --inspect-brk-node to work, does that refer to breaking on the first line of Node.js code, or first line of Electron code? Or does that not matter here?)

[codebytere]

@addaleax coming back after some more investigation - it does actually look like what we need is to prevent the inspector from being initialized in some cases. I've wrangled together a PR over in Electron that removes our custom debugger 🥳

However when we have nodeIntegration enabled in the renderer process, Blink creates the V8 inspector themselves, so that means two of them will be created, leading to a fun set of crashes.

Happy to shift this around as preferred, but the specific goal we do need to address is the the ability to prevent env->InitializeInspector from always being called.

[codebytere]

@addaleax what do you think about this PR conceptually given new information above?

[addaleax]

@codebytere Sorry, I didn’t notice the ping above!

I think this feature sounds fine conceptually, but most of my concerns still stand … it should be documented differently, without referencing InitializeInspector() or InspectorAgent, and probably rather stating that the flag keeps Node.js from calling V8Inspector::create() – I assume that that is the part that Electron ultimately cares about as well.

But maybe more importantly, some of the code in Node.js does not expect an uninitialized InspectorAgent – if I take this PR, hardcode Environment::should_initialize_inspector() to set false, and run the tests, there are a few crashes that I think we should probably still take care of (which is also probably in Electron’s interest here 🙂 ):

Test output in the fold

=== release test-child-process-spawnsync-kill-signal ===                      
Path: parallel/test-child-process-spawnsync-kill-signal
assert.js:103
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected
+ 'SIGKILL'
- 'SIGTERM'
      ^
    at Object.<anonymous> (/Users/addaleax/src/node/test/parallel/test-child-process-spawnsync-kill-signal.js:42:12)
    at Module._compile (internal/modules/cjs/loader.js:1090:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1111:10)
    at Module.load (internal/modules/cjs/loader.js:955:32)
    at Function.Module._load (internal/modules/cjs/loader.js:796:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 'SIGKILL',
  expected: 'SIGTERM',
  operator: 'strictEqual'
}
Command: out/Release/node --expose-internals /Users/addaleax/src/node/test/parallel/test-child-process-spawnsync-kill-signal.js
=== release test-inspector-async-hook-after-done ===                          
Path: parallel/test-inspector-async-hook-after-done
Worker is done
assert.js:103
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
false !== true
    at Timeout._onTimeout (/Users/addaleax/src/node/test/parallel/test-inspector-async-hook-after-done.js:59:16)
    at listOnTimeout (internal/timers.js:555:17)
    at processTimers (internal/timers.js:498:7) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: 'strictEqual'
}
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-inspector-async-hook-after-done.js
=== release test-inspector-connect-main-thread ===                        
Path: parallel/test-inspector-connect-main-thread
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-inspector-connect-main-thread.js
--- CRASHED (Signal: 11) ---
=== release test-inspector-workers-flat-list ===                              
Path: parallel/test-inspector-workers-flat-list
Worker 1 is running
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-inspector-workers-flat-list.js
--- CRASHED (Signal: 11) ---
=== release test-inspector-heap-allocation-tracker ===                        
Path: parallel/test-inspector-heap-allocation-tracker
out/Release/node[90973]: ../src/inspector_agent.cc:755:std::unique_ptr<InspectorSession> node::inspector::Agent::Connect(std::unique_ptr<InspectorSessionDelegate>, bool): Assertion `(client_) != nullptr' failed.
 1: 0x1000aef65 node::Abort() [/Users/addaleax/src/node/out/Release/node]
 2: 0x1000aedd1 node::Assert(node::AssertionInfo const&) [/Users/addaleax/src/node/out/Release/node]
 3: 0x100180297 node::inspector::Agent::Connect(std::__1::unique_ptr<node::inspector::InspectorSessionDelegate, std::__1::default_delete<node::inspector::InspectorSessionDelegate> >, bool) [/Users/addaleax/src/node/out/Release/node]
 4: 0x10018a997 node::inspector::(anonymous namespace)::JSBindingsConnection<node::inspector::(anonymous namespace)::LocalConnection>::New(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/addaleax/src/node/out/Release/node]
 5: 0x100260c0d v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/Users/addaleax/src/node/out/Release/node]
 6: 0x10025febc v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/addaleax/src/node/out/Release/node]
 7: 0x10025f8af v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/addaleax/src/node/out/Release/node]
 8: 0x100a66399 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit [/Users/addaleax/src/node/out/Release/node]
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-inspector-heap-allocation-tracker.js
--- CRASHED (Signal: 6) ---
=== release test-trace-events-dynamic-enable-workers-disabled ===             
Path: parallel/test-trace-events-dynamic-enable-workers-disabled
out/Release/node[92239]: ../src/inspector_agent.cc:755:std::unique_ptr<InspectorSession> node::inspector::Agent::Connect(std::unique_ptr<InspectorSessionDelegate>, bool): Assertion `(client_) != nullptr' failed.
 1: 0x1000aef65 node::Abort() [/Users/addaleax/src/node/out/Release/node]
 2: 0x1000aedd1 node::Assert(node::AssertionInfo const&) [/Users/addaleax/src/node/out/Release/node]
 3: 0x100180297 node::inspector::Agent::Connect(std::__1::unique_ptr<node::inspector::InspectorSessionDelegate, std::__1::default_delete<node::inspector::InspectorSessionDelegate> >, bool) [/Users/addaleax/src/node/out/Release/node]
 4: 0x10018a997 node::inspector::(anonymous namespace)::JSBindingsConnection<node::inspector::(anonymous namespace)::LocalConnection>::New(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/addaleax/src/node/out/Release/node]
 5: 0x100260c0d v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/Users/addaleax/src/node/out/Release/node]
 6: 0x10025febc v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/addaleax/src/node/out/Release/node]
 7: 0x10025f8af v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/addaleax/src/node/out/Release/node]
 8: 0x100a66399 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit [/Users/addaleax/src/node/out/Release/node]
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-trace-events-dynamic-enable-workers-disabled.js
--- CRASHED (Signal: 6) ---
=== release test-v8-coverage ===                                              
Path: parallel/test-v8-coverage
assert.js:385
    throw err;
    ^
AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
  assert.ok(fixtureCoverage)
    at Object.<anonymous> (/Users/addaleax/src/node/test/parallel/test-v8-coverage.js:129:10)
    at Module._compile (internal/modules/cjs/loader.js:1090:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1111:10)
    at Module.load (internal/modules/cjs/loader.js:955:32)
    at Function.Module._load (internal/modules/cjs/loader.js:796:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: true,
  operator: '=='
}
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-v8-coverage.js
=== release test-worker-debug ===                                             
Path: parallel/test-worker-debug
Test basic debug scenario
This is a message from a worker
Mismatched noop function calls. Expected exactly 1, actual 0.
    at Proxy.mustCall (/Users/addaleax/src/node/test/common/index.js:329:10)
    at Object.<anonymous> (/Users/addaleax/src/node/test/parallel/test-worker-debug.js:281:18)
    at Module._compile (internal/modules/cjs/loader.js:1090:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1111:10)
    at Module.load (internal/modules/cjs/loader.js:955:32)
    at Function.Module._load (internal/modules/cjs/loader.js:796:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-worker-debug.js
=== release test-worker-nested-on-process-exit ===                            
Path: parallel/test-worker-nested-on-process-exit
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-worker-nested-on-process-exit.js
--- CRASHED (Signal: 11) ---
=== release test-worker-nested-uncaught ===                             
Path: parallel/test-worker-nested-uncaught
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-worker-nested-uncaught.js
--- CRASHED (Signal: 11) ---
=== release test-worker-process-cwd ===                                       
Path: parallel/test-worker-process-cwd
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-worker-process-cwd.js
--- CRASHED (Signal: 11) ---
=== release test-worker-terminate-nested ===                                  
Path: parallel/test-worker-terminate-nested
Command: out/Release/node /Users/addaleax/src/node/test/parallel/test-worker-terminate-nested.js
--- CRASHED (Signal: 11) ---
=== release test-cpu-prof-dir-worker ===                                      
Path: sequential/test-cpu-prof-dir-worker
assert.js:103
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
1 !== 2
    at Object.<anonymous> (/Users/addaleax/src/node/test/sequential/test-cpu-prof-dir-worker.js:43:10)
    at Module._compile (internal/modules/cjs/loader.js:1090:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1111:10)
    at Module.load (internal/modules/cjs/loader.js:955:32)
    at Function.Module._load (internal/modules/cjs/loader.js:796:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 1,
  expected: 2,
  operator: 'strictEqual'
}
Command: out/Release/node /Users/addaleax/src/node/test/sequential/test-cpu-prof-dir-worker.js
=== release test-cpu-prof-worker-argv ===                        
Path: sequential/test-cpu-prof-worker-argv
assert.js:103
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
0 !== 1
    at Object.<anonymous> (/Users/addaleax/src/node/test/sequential/test-cpu-prof-worker-argv.js:36:10)
    at Module._compile (internal/modules/cjs/loader.js:1090:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1111:10)
    at Module.load (internal/modules/cjs/loader.js:955:32)
    at Function.Module._load (internal/modules/cjs/loader.js:796:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 0,
  expected: 1,
  operator: 'strictEqual'
}
Command: out/Release/node /Users/addaleax/src/node/test/sequential/test-cpu-prof-worker-argv.js
=== release test-heap-prof ===                                      
Path: sequential/test-heap-prof
assert.js:103
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
0 !== 1
    at Object.<anonymous> (/Users/addaleax/src/node/test/sequential/test-heap-prof.js:173:10)
    at Module._compile (internal/modules/cjs/loader.js:1090:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1111:10)
    at Module.load (internal/modules/cjs/loader.js:955:32)
    at Function.Module._load (internal/modules/cjs/loader.js:796:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: 0,
  expected: 1,
  operator: 'strictEqual'
}
Command: out/Release/node /Users/addaleax/src/node/test/sequential/test-heap-prof.js
[02:12|% 100|+ 3021|-  15]: Done      

[codebytere]

I think that should do it - i updated the documentation and added some error coverage such that a missing inspector will cause more descriptive errors to be thrown instead of crashes occurring.

[addaleax]

Yup, and thank you for bearing with me here :)

[joyeecheung]

It would be nicer if this error is defined in node_errors.h and documented in errors.md, though I understand that this would not normally show up when you use the Node.js official build so it's more like something that belongs to the doc of the embedder..

[codebytere]

@Trott yes please! i can handle it if it looks author-ready :)

[Trott]

@Trott yes please! i can handle it if it looks author-ready :)

I've added the commit-queue label so hopefully it lands cleanly through the automated process in the next 10 minutes or so.

[nodejs-github-bot]

Landed in 0367b5c