Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure the inspector agent is shutdown before cleaning up the node env #18076

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions atom/app/node_main.cc
Expand Up @@ -96,6 +96,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());
Expand Down
1 change: 1 addition & 0 deletions atom/browser/atom_browser_main_parts.cc
Expand Up @@ -481,6 +481,7 @@ void AtomBrowserMainParts::PostMainMessageLoopRun() {
ui::SetX11ErrorHandlers(X11EmptyErrorHandler, X11EmptyIOErrorHandler);
#endif

node_debugger_->Stop();
js_env_->OnMessageLoopDestroying();

#if defined(OS_MACOSX)
Expand Down
6 changes: 6 additions & 0 deletions atom/browser/node_debugger.cc
Expand Up @@ -59,4 +59,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
1 change: 1 addition & 0 deletions atom/browser/node_debugger.h
Expand Up @@ -20,6 +20,7 @@ class NodeDebugger {
~NodeDebugger();

void Start();
void Stop();

private:
node::Environment* env_;
Expand Down
3 changes: 3 additions & 0 deletions spec/fixtures/module/delay-exit.js
@@ -0,0 +1,3 @@
const { app } = require('electron')

process.on('message', () => app.quit())
45 changes: 43 additions & 2 deletions spec/node-spec.js
Expand Up @@ -8,6 +8,8 @@ const os = require('os')
const { ipcRenderer, remote } = require('electron')
const features = process.atomBinding('features')

const { emittedOnce } = require('./events-helpers')

const isCI = remote.getGlobal('isCi')
chai.use(dirtyChai)

Expand Down Expand Up @@ -219,15 +221,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) => {
Expand Down Expand Up @@ -263,6 +272,7 @@ describe('node feature', () => {
ELECTRON_RUN_AS_NODE: true
}
})
exitPromise = emittedOnce(child, 'exit')

let output = ''
function cleanup () {
Expand All @@ -287,6 +297,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) {
Expand All @@ -303,13 +314,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') {
Expand Down