fix: crash with --inspect-brk when running under ELECTRON_RUN_AS_NODE (electron#20098)

deepak1556 · web-flow · commit 4662ab9b63aa · 2019-09-04T12:45:50.000-07:00
* fix: ensure that the node env is not bootstrapped before running inspector

* fix: ensure we wait for the inspect to disconnect

This re-orders our node clean up so that we free the environment before
the task runner is cleaned up as node uses the task runner during clean
up.  It also calls WaitForDisconnect() to ensure that inspector agents
are notified that the context is going down.

* chore: update spec to catch SIGABRT
diff --git a/atom/app/node_main.cc b/atom/app/node_main.cc
@@ -47,15 +47,6 @@ int NodeMain(int argc, char* argv[]) {
     feature_list->InitializeFromCommandLine("", "");
     base::FeatureList::SetInstance(std::move(feature_list));
 
-    gin::V8Initializer::LoadV8Snapshot(
-        gin::V8Initializer::V8SnapshotFileType::kWithAdditionalContext);
-    gin::V8Initializer::LoadV8Natives();
-
-    // V8 requires a task scheduler apparently
-    base::ThreadPoolInstance::CreateAndStartWithDefaultParams("Electron");
-
-    // Initialize gin::IsolateHolder.
-    JavascriptEnvironment gin_env(loop);
 #if defined(_WIN64)
     crash_reporter::CrashReporterWin::SetUnhandledExceptionFilter();
 #endif
@@ -67,14 +58,26 @@ int NodeMain(int argc, char* argv[]) {
     const char** exec_argv;
     node::Init(&argc, const_cast<const char**>(argv), &exec_argc, &exec_argv);
 
+    gin::V8Initializer::LoadV8Snapshot(
+        gin::V8Initializer::V8SnapshotFileType::kWithAdditionalContext);
+    gin::V8Initializer::LoadV8Natives();
+
+    // V8 requires a task scheduler apparently
+    base::ThreadPoolInstance::CreateAndStartWithDefaultParams("Electron");
+
+    // Initialize gin::IsolateHolder.
+    JavascriptEnvironment gin_env(loop);
+
     node::Environment* env = node::CreateEnvironment(
         node::CreateIsolateData(gin_env.isolate(), loop, gin_env.platform()),
-        gin_env.context(), argc, argv, exec_argc, exec_argv);
+        gin_env.context(), argc, argv, exec_argc, exec_argv, false);
 
     // Enable support for v8 inspector.
     NodeDebugger node_debugger(env);
     node_debugger.Start();
 
+    node::BootstrapEnvironment(env);
+
     mate::Dictionary process(gin_env.isolate(), env->process_object());
 #if defined(OS_WIN)
     process.SetMethod("log", &ElectronBindings::Log);
@@ -110,12 +113,15 @@ int NodeMain(int argc, char* argv[]) {
 
     node_debugger.Stop();
     exit_code = node::EmitExit(env);
+    env->set_can_call_into_js(false);
     node::RunAtExit(env);
-    gin_env.platform()->DrainTasks(env->isolate());
-    gin_env.platform()->CancelPendingDelayedTasks(env->isolate());
-    gin_env.platform()->UnregisterIsolate(env->isolate());
 
+    v8::Isolate* isolate = env->isolate();
     node::FreeEnvironment(env);
+
+    gin_env.platform()->DrainTasks(isolate);
+    gin_env.platform()->CancelPendingDelayedTasks(isolate);
+    gin_env.platform()->UnregisterIsolate(isolate);
   }
 
   // According to "src/gin/shell/gin_main.cc":
diff --git a/atom/browser/node_debugger.cc b/atom/browser/node_debugger.cc
@@ -60,8 +60,10 @@ void NodeDebugger::Start() {
 
 void NodeDebugger::Stop() {
   auto* inspector = env_->inspector_agent();
-  if (inspector && inspector->IsListening())
+  if (inspector && inspector->IsListening()) {
+    inspector->WaitForDisconnect();
     inspector->Stop();
+  }
 }
 
 }  // namespace atom
diff --git a/spec/node-spec.js b/spec/node-spec.js
@@ -311,13 +311,20 @@ describe('node feature', () => {
         output += data
         if (output.trim().startsWith('Debugger listening on ws://')) {
           cleanup()
-          done()
+          child.kill('SIGTERM')
         }
       }
       function outDataHandler (data) {
         cleanup()
         done(new Error(`Unexpected output: ${data.toString()}`))
       }
+      child.on('close', (code, signal) => {
+        if (signal !== 'SIGTERM') {
+          done(new Error(`Unexpected termination with code: ${code} and signal: ${signal}`))
+        } else {
+          done()
+        }
+      })
       child.stderr.on('data', errorDataListener)
       child.stdout.on('data', outDataHandler)
     })
@@ -390,6 +397,7 @@ describe('node feature', () => {
             const connection = new WebSocket(socketMatch[0])
             connection.onopen = () => {
               child.send('plz-quit')
+              connection.close()
               done()
             }
           }

Original file line number	Diff line number	Diff line change
`@@ -311,13 +311,20 @@ describe('node feature', () => {`
`311`	`311`	`output += data`
`312`	`312`	`if (output.trim().startsWith('Debugger listening on ws://')) {`
`313`	`313`	`cleanup()`
`314`		`- done()`
	`314`	`+ child.kill('SIGTERM')`
`315`	`315`	`}`
`316`	`316`	`}`
`317`	`317`	`function outDataHandler (data) {`
`318`	`318`	`cleanup()`
`319`	`319`	done(new Error(`Unexpected output: ${data.toString()}`))
`320`	`320`	`}`
	`321`	`+ child.on('close', (code, signal) => {`
	`322`	`+ if (signal !== 'SIGTERM') {`
	`323`	+ done(new Error(`Unexpected termination with code: ${code} and signal: ${signal}`))
	`324`	`+ } else {`
	`325`	`+ done()`
	`326`	`+ }`
	`327`	`+ })`
`321`	`328`	`child.stderr.on('data', errorDataListener)`
`322`	`329`	`child.stdout.on('data', outDataHandler)`
`323`	`330`	`})`
`@@ -390,6 +397,7 @@ describe('node feature', () => {`
`390`	`397`	`const connection = new WebSocket(socketMatch[0])`
`391`	`398`	`connection.onopen = () => {`
`392`	`399`	`child.send('plz-quit')`
	`400`	`+ connection.close()`
`393`	`401`	`done()`
`394`	`402`	`}`
`395`	`403`	`}`