src: fix InspectorStarted macro guard

[danbev]

Currently the InspectorStarted function is guarded by the else clause of
the NODE_USE_V8_PLATFORM macro. If node is configured --without-ssl then
NODE_USE_V8_PLATFORM will be 1 but the nested HAVE_INSPECTOR macro
will not be 0 which will lead to that there will be no InspectorStarted
function defined.

If building --without-inspector or --without-ssl the following
compilation error will occur:

../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
1 error generated.

This commit adds a separate if preprocessor directive to catch the case
when either --without-ssl/--without-inspector and --without-v8-platform
combinations are used to configure node.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

[danbev]

CI: https://ci.nodejs.org/job/node-test-pull-request/8244/

[bnoordhuis]

Random observations about the src/node.cc in master:

1. The call to v8_platform.StartInspector() is guarded by a #ifdef HAVE_INSPECTOR but the call to v8_platform.InspectorStarted() is not.

2. The v8_platform.StartInspector() for the NODE_USE_V8_PLATFORM == 0 case is never called because of point 1.

3. There are several (guarded) calls to env->inspector_agent()->IsStarted(), there is only one call to v8_platform.InspectorStarted(). The patch below reduces the line count and is arguably more straightforward.

4. The logic in node.gyp and node.gypi seems wrong: if the inspector depends on libplatform, then node_use_v8_platform=="false" should imply v8_enable_inspector==0 - i.e., if NODE_USE_V8_PLATFORM == 0, then logically HAVE_INSPECTOR == 0.

I think if you enforce 4, you'll be able to cut quite a bit of code and ifdef logic. Good idea, bad idea?

diff --git a/src/node.cc b/src/node.cc
index a5c86d4..81e1dd7 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -260,10 +260,6 @@ static struct {
                       const node::DebugOptions& options) {
     return env->inspector_agent()->Start(platform_, script_path, options);
   }
-
-  bool InspectorStarted(Environment *env) {
-    return env->inspector_agent()->IsStarted();
-  }
 #endif  // HAVE_INSPECTOR
 
   void StartTracingAgent() {
@@ -293,9 +289,6 @@ static struct {
                     "so event tracing is not available.\n");
   }
   void StopTracingAgent() {}
-  bool InspectorStarted(Environment *env) {
-    return false;
-  }
 #endif  // !NODE_USE_V8_PLATFORM
 } v8_platform;
 
@@ -4467,8 +4460,13 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   const char* path = argc > 1 ? argv[1] : nullptr;
   StartDebug(&env, path, debug_options);
 
-  if (debug_options.inspector_enabled() && !v8_platform.InspectorStarted(&env))
-    return 12;  // Signal internal error.
+  if (debug_options.inspector_enabled()) {
+#if HAVE_INSPECTOR
+    if (!env->inspector_agent()->IsStarted()) return 12;  // Internal error.
+#else
+    return 12;  // Internal error.
+#endif
+  }
 
   env.set_abort_on_uncaught_exception(abort_on_uncaught_exception);
 

[danbev]

I think if you enforce 4, you'll be able to cut quite a bit of code and ifdef logic. Good idea, bad idea?

Sounds good to me. I'll take a look at this.

Just for my understanding of when --without-v8-platform would be used. When configured, the responsibility of setting the platform (v8::V8::InitializePlatform(platform_);) is up to the caller and building node --without-v8-platform is expected to fail with error from V8 stating this. I'm guessing this is use by someone embedding node, perhaps Electron?

[refack]

shouldn't it be #if !defined(NODE_USE_V8_PLATFORM) || !defined(HAVE_INSPECTOR)?

[danbev]

I thought so too, but we need to check the value as the names will always be defined as macros.

[refack]

sorry to nag, but what about #if !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR

[danbev]

sorry to nag, but what about #if !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR

Yes, that works as it is equivalent to the current statement. I'm fine with either.

[danbev]

I'll make this change.

[bnoordhuis]

Just for my understanding of when --without-v8-platform would be used. When configured, the responsibility of setting the platform (v8::V8::InitializePlatform(platform_);) is up to the caller and building node --without-v8-platform is expected to fail with error from V8 stating this. I'm guessing this is use by someone embedding node, perhaps Electron?

Yes, correct.

[bnoordhuis]

This PR LGTM as a stop-gap measure, by the way.

[kfarnung]

Ran into this problem in my fork and had a similar fix in mind. 👍

[danbev]

If there are no objections I'd like to merge this and then follow up with a separate PR when I have a little more time (hopefully next week) to address the comments from @bnoordhuis.

[danbev]

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/8318/

[danbev]

Landed in ae5e65c

[MylesBorins]

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land