From 5f550234c4e2ae92479aa23c606f693b03eb0039 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 30 Apr 2019 15:44:40 -0700 Subject: [PATCH] fix: ensure the inspector agent is shutdown before cleaning up the node env (#18028) * 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 --- atom/app/node_main.cc | 1 + atom/browser/atom_browser_main_parts.cc | 1 + atom/browser/node_debugger.cc | 6 ++++ atom/browser/node_debugger.h | 1 + spec/fixtures/module/delay-exit.js | 3 ++ spec/node-spec.js | 45 +++++++++++++++++++++++-- 6 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 spec/fixtures/module/delay-exit.js diff --git a/atom/app/node_main.cc b/atom/app/node_main.cc index c9ef9a7d4a363..82c026e8812c9 100644 --- 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 index 27abd53bfdc43..da470d3c34c42 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -452,6 +452,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 index 1a47e81fdd968..71a06140496fe 100644 --- 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 index be69f9cfd18e1..eb173d6908a0d 100644 --- 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 new file mode 100644 index 0000000000000..fa7895449b32f --- /dev/null +++ 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 index 3c2616141a612..623ecacd2135d 100644 --- 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,6 +233,7 @@ describe('node feature', () => { describe('inspector', () => { let child = null + let exitPromise = null beforeEach(function () { if (!features.isRunAsNodeEnabled()) { @@ -238,8 +241,14 @@ describe('node feature', () => { } }) - 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,6 +326,35 @@ 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: { @@ -322,6 +362,7 @@ describe('node feature', () => { }, stdio: ['ipc'] }) + exitPromise = emittedOnce(child, 'exit') child.on('message', ({ cmd, debuggerEnabled, success }) => { if (cmd === 'assert') {