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

inspector: crash on debugger connection if JS session is already active #16852

Closed
ofrobots opened this issue Nov 6, 2017 · 5 comments
Closed
Assignees
Labels
inspector Issues and PRs related to the V8 inspector protocol

Comments

@ofrobots
Copy link
Contributor

ofrobots commented Nov 6, 2017

Found this on googleapis/cloud-debug-nodejs#356 originally.

// Flags: --inspect=9229
const inspector = require('inspector');
const session = new inspector.Session();
session.connect();

setInterval(() => { console.log('jello')}, 2000);

setTimeout(() => {
  require('child_process').spawn(process.execPath, [
    'inspect',
    'localhost:9229'
  ])
}, 1000);

Crashes with:

$ ./node_g --inspect ~/tmp/test/test.js
Debugger listening on ws://127.0.0.1:9229/8c72c164-5fc4-41b9-af56-8a8c4cf95cb7
For help see https://nodejs.org/en/docs/inspector
jello
Debugger attached.
/Users/ofrobots/src/node/out/Debug/node[35110]: ../src/inspector_agent.cc:347:void node::inspector::NodeInspectorClient::connectFrontend(node::inspector::InspectorSessionDelegate *): Assertion `(channel_) == (nullptr)' failed.
 1: node::Abort() [/Users/ofrobots/src/node/./node_g]
 2: node::(anonymous namespace)::DomainEnter(node::Environment*, v8::Local<v8::Object>) [/Users/ofrobots/src/node/./node_g]
 3: node::inspector::NodeInspectorClient::connectFrontend(node::inspector::InspectorSessionDelegate*) [/Users/ofrobots/src/node/./node_g]
 4: node::inspector::Agent::Connect(node::inspector::InspectorSessionDelegate*) [/Users/ofrobots/src/node/./node_g]
 5: node::inspector::InspectorIo::DispatchMessages() [/Users/ofrobots/src/node/./node_g]
 6: node::inspector::DispatchMessagesTask::Run() [/Users/ofrobots/src/node/./node_g]
 7: node::RunForegroundTask(v8::Task*) [/Users/ofrobots/src/node/./node_g]
 8: node::NodePlatform::FlushForegroundTasksInternal() [/Users/ofrobots/src/node/./node_g]
 9: node::FlushTasks(uv_async_s*) [/Users/ofrobots/src/node/./node_g]
10: uv__async_io [/Users/ofrobots/src/node/./node_g]
11: uv__io_poll [/Users/ofrobots/src/node/./node_g]
12: uv_run [/Users/ofrobots/src/node/./node_g]
13: node::Start(v8::Isolate*, node::IsolateData*, int, char const* const*, int, char const* const*) [/Users/ofrobots/src/node/./node_g]
14: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [/Users/ofrobots/src/node/./node_g]
15: node::Start(int, char**) [/Users/ofrobots/src/node/./node_g]
16: main [/Users/ofrobots/src/node/./node_g]
17: start [/Users/ofrobots/src/node/./node_g]
[1]    35110 abort      ./node_g --inspect ~/tmp/test/test.js

This affects master, 9.x and 8.x.

/cc @nodejs/v8-inspector

@TimothyGu
Copy link
Member

TimothyGu commented Nov 7, 2017

Seems to be a duplicate of #14745. Specifically #14745 (comment)

@eugeneo eugeneo self-assigned this Nov 7, 2017
@auchenberg
Copy link

@ofrobots @eugeneo What should happen here? Should the port automatically be incremented? I assume it crashes as port 9922 is already used by the main process?

@eugeneo
Copy link
Contributor

eugeneo commented Nov 7, 2017

This is a bug in the Node inspector code. There is an assertion thrown when WS session is connected while there's a JS bindings session.

I tried to fix it in the past, but it requires a rework of the WS connection process that is currently happening on another thread but now will need a roundtrip to the main thread.

Chrome is currently launching support for the multiple concurrent Inspector sessions. Once that support is stable and Node moves on a compatible V8, this assertion will be removed. There will likely be a need to do some other work in Node.js to support concurrent sessions - but at least this bug will be auto-fixed.

@eugeneo
Copy link
Contributor

eugeneo commented Nov 7, 2017

I am still planning to take another look at this bug later this week, time permitting - I think there might be a reasonable fix.

MylesBorins pushed a commit that referenced this issue Dec 12, 2017
Attaching WS session will now include a roundtrip onto the main thread
to make sure there is no other session (e.g. JS bindings)

This change also required refactoring WS socket implementation to better
support scenarios like this.

Fixes: #16852
PR-URL: #17085
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
Attaching WS session will now include a roundtrip onto the main thread
to make sure there is no other session (e.g. JS bindings)

This change also required refactoring WS socket implementation to better
support scenarios like this.

Fixes: #16852
PR-URL: #17085
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@heisian
Copy link

heisian commented Jul 13, 2018

Node is wonderful, we get the opportunity to debug the debugger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

No branches or pull requests

5 participants