fix: ensure the inspector agent is shutdown before cleaning up the node env (electron#18077)

trop[bot] · codebytere · commit be16a195fb3d · 2019-05-01T13:47:22.000-07:00
* fix: ensure the inspector agent is shutdown before cleaning up the node env

* spec: add tests to ensure clean shutdown with connected inspector agent

* Update node_debugger.cc
diff --git a/atom/app/node_main.cc b/atom/app/node_main.cc
@@ -101,6 +101,7 @@ int NodeMain(int argc, char* argv[]) {
       }
     } while (more == true);
 
+    node_debugger.Stop();
     exit_code = node::EmitExit(env);
     node::RunAtExit(env);
     gin_env.platform()->DrainTasks(env->isolate());
diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc
@@ -469,6 +469,7 @@ void AtomBrowserMainParts::PostMainMessageLoopRun() {
   ui::SetX11ErrorHandlers(X11EmptyErrorHandler, X11EmptyIOErrorHandler);
 #endif
 
+  node_debugger_->Stop();
   js_env_->OnMessageLoopDestroying();
 
 #if defined(OS_MACOSX)
diff --git a/atom/browser/node_debugger.cc b/atom/browser/node_debugger.cc
@@ -58,4 +58,10 @@ void NodeDebugger::Start() {
     DCHECK(env_->inspector_agent()->IsListening());
 }
 
+void NodeDebugger::Stop() {
+  auto* inspector = env_->inspector_agent();
+  if (inspector && inspector->IsListening())
+    inspector->Stop();
+}
+
 }  // namespace atom
diff --git a/atom/browser/node_debugger.h b/atom/browser/node_debugger.h
@@ -20,6 +20,7 @@ class NodeDebugger {
   ~NodeDebugger();
 
   void Start();
+  void Stop();
 
  private:
   node::Environment* env_;
diff --git a/spec/fixtures/module/delay-exit.js b/spec/fixtures/module/delay-exit.js
@@ -0,0 +1,3 @@
+const { app } = require('electron')
+
+process.on('message', () => app.quit())
diff --git a/spec/node-spec.js b/spec/node-spec.js
@@ -8,6 +8,8 @@ const os = require('os')
 const { ipcRenderer, remote } = require('electron')
 const features = process.electronBinding('features')
 
+const { emittedOnce } = require('./events-helpers')
+
 const isCI = remote.getGlobal('isCi')
 chai.use(dirtyChai)
 
@@ -231,15 +233,22 @@ describe('node feature', () => {
 
   describe('inspector', () => {
     let child = null
+    let exitPromise = null
 
     beforeEach(function () {
       if (!features.isRunAsNodeEnabled()) {
         this.skip()
       }
     })
 
-    afterEach(() => {
-      if (child !== null) child.kill()
+    afterEach(async () => {
+      if (child && exitPromise) {
+        const [code, signal] = await exitPromise
+        expect(signal).to.equal(null)
+        expect(code).to.equal(0)
+      } else if (child) {
+        child.kill()
+      }
     })
 
     it('supports starting the v8 inspector with --inspect/--inspect-brk', (done) => {
@@ -275,6 +284,7 @@ describe('node feature', () => {
           ELECTRON_RUN_AS_NODE: true
         }
       })
+      exitPromise = emittedOnce(child, 'exit')
 
       let output = ''
       function cleanup () {
@@ -299,6 +309,7 @@ describe('node feature', () => {
 
     it('does not start the v8 inspector when --inspect is after a -- argument', (done) => {
       child = ChildProcess.spawn(remote.process.execPath, [path.join(__dirname, 'fixtures', 'module', 'noop.js'), '--', '--inspect'])
+      exitPromise = emittedOnce(child, 'exit')
 
       let output = ''
       function dataListener (data) {
@@ -315,13 +326,43 @@ describe('node feature', () => {
       })
     })
 
+    it('does does not crash when quitting with the inspector connected', function (done) {
+      // IPC Electron child process not supported on Windows
+      if (process.platform === 'win32') return this.skip()
+      child = ChildProcess.spawn(remote.process.execPath, [path.join(__dirname, 'fixtures', 'module', 'delay-exit'), '--inspect=0'], {
+        stdio: ['ipc']
+      })
+      exitPromise = emittedOnce(child, 'exit')
+
+      let output = ''
+      function dataListener (data) {
+        output += data
+
+        if (output.trim().startsWith('Debugger listening on ws://') && output.endsWith('\n')) {
+          const socketMatch = output.trim().match(/(ws:\/\/.+:[0-9]+\/.+?)\n/gm)
+          if (socketMatch && socketMatch[0]) {
+            child.stderr.removeListener('data', dataListener)
+            child.stdout.removeListener('data', dataListener)
+            const connection = new WebSocket(socketMatch[0])
+            connection.onopen = () => {
+              child.send('plz-quit')
+              done()
+            }
+          }
+        }
+      }
+      child.stderr.on('data', dataListener)
+      child.stdout.on('data', dataListener)
+    })
+
     it('supports js binding', (done) => {
       child = ChildProcess.spawn(process.execPath, ['--inspect', path.join(__dirname, 'fixtures', 'module', 'inspector-binding.js')], {
         env: {
           ELECTRON_RUN_AS_NODE: true
         },
         stdio: ['ipc']
       })
+      exitPromise = emittedOnce(child, 'exit')
 
       child.on('message', ({ cmd, debuggerEnabled, success }) => {
         if (cmd === 'assert') {

Original file line number	Diff line number	Diff line change
`@@ -101,6 +101,7 @@ int NodeMain(int argc, char* argv[]) {`
`101`	`101`	`}`
`102`	`102`	`} while (more == true);`
`103`	`103`
	`104`	`+ node_debugger.Stop();`
`104`	`105`	`exit_code = node::EmitExit(env);`
`105`	`106`	`node::RunAtExit(env);`
`106`	`107`	`gin_env.platform()->DrainTasks(env->isolate());`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+const { app } = require('electron')`
	`2`	`+`
	`3`	`+process.on('message', () => app.quit())`