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

webSocketDebuggerUrl returned by v8_inspector in /json is invalid #7227

Closed
auchenberg opened this issue Jun 8, 2016 · 9 comments · Fixed by #7232
Closed

webSocketDebuggerUrl returned by v8_inspector in /json is invalid #7227

auchenberg opened this issue Jun 8, 2016 · 9 comments · Fixed by #7232
Labels
inspector Issues and PRs related to the V8 inspector protocol

Comments

@auchenberg
Copy link

auchenberg commented Jun 8, 2016

The webSocketDebuggerUrl returned by v8_inspector in /json HTTP endpoint is invalid, and causes 3rd party clients like VS Code and Sublime to fail connecting to the new CDP endpoint.

In addition then devtoolsFrontendUrl is served without the ?ws= querystring, which is returned by Chrome and Edge.

  • Version: v7.0.0-pre
  • Platform: OSX
  • Subsystem: v8_inspector

Actual response from http://localhost:5858/json:

[{
    "description": "node.js instance",
    "devtoolsFrontendUrl": "https://chrome-devtools-frontend.appspot.com/serve_file/@4604d24a75168768584760ba56d175507941852f/inspector.html",
    "faviconUrl": "https://nodejs.org/static/favicon.ico",
    "id": "27382",
    "title": "./node",
    "type": "node",
    "webSocketDebuggerUrl": "ws:///node"
}]

Expected response:

[{
    "description": "node.js instance",
    "devtoolsFrontendUrl": "https://chrome-devtools-frontend.appspot.com/serve_file/@4604d24a75168768584760ba56d175507941852f/inspector.html?experiments=true&v8only=true&ws=localhost:5858/node",
    "faviconUrl": "https://nodejs.org/static/favicon.ico",
    "id": "27382",
    "title": "./node",
    "type": "node",
    "webSocketDebuggerUrl": "ws://localhost:5858/node"
}]

(CC @ofrobots, @pavelfeldman)

@MylesBorins
Copy link
Member

@nodejs/diagnostics

@MylesBorins MylesBorins added the inspector Issues and PRs related to the V8 inspector protocol label Jun 8, 2016
@pavelfeldman
Copy link
Contributor

Yes, it is a bug in inspector_agent.cc. Would you be able to send a PR?

@MylesBorins
Copy link
Member

@pavelfeldman would this be against the v8 repo? @auchenberg if you do not have time to do this I'd be more than happy to, just let me know

@pavelfeldman
Copy link
Contributor

@pavelfeldman
Copy link
Contributor

It'd be nice if we could extract hash into a const and reuse it as well. I can see that entire block is wrong. Should be using same hash as https://github.com/nodejs/node/blob/master/src/inspector_agent.cc#L37

@MylesBorins
Copy link
Member

MylesBorins commented Jun 8, 2016

@pavelfeldman I'm digging into this right now. It seems like it isn't going to be straight forward afaict as SendTargentsListResponse and a variety of callbacks that call it are all static. At the moment I cannot find a way to get the port in the static function.

The call stack

SendTargentsListResponse (static)
RespondToGet (static)
Agent::OnInspectorHandshakeIO (static)
Agent::OnSocketConnectionIO (static) (last place we have access to uv_stream_t* server)
Agent::WorkerRunIO (first non static function in call stack, have access to port_)

Agent::OnSocketConnectionIO is passed to uv_listen so that is likely the end of the line here as to where we can do anything. I'm digging through the libuv docs right now but I'm not entirely sure if it is possible to get the port from uv_stream_t or not... I'm going to continue digging but would appreciate any help

edit: it is worth mentioning that the instance of uv_stream_t is a recast instance of uv_tcp_t

@pavelfeldman
Copy link
Contributor

Look at how Agent::OnRemoteDataIO unwraps Agent from stream:

  inspector_socket_t* socket = static_cast<inspector_socket_t*>(stream->data);
  Agent* agent = static_cast<Agent*>(socket->data);

@MylesBorins
Copy link
Member

@pavelfeldman thanks! figured it out and pushed a PR

MylesBorins pushed a commit to MylesBorins/node that referenced this issue Jun 9, 2016
Fix the `webSocketDebuggerUrl` and `devtoolsFrontendUrl` returned by
v8_inspector in /json HTTP endpoint to work with 3rd party clients.

Fixes: nodejs#7227
PR-URL: nodejs#7232
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@nojvek
Copy link

nojvek commented Jun 25, 2016

Not sure if someone's already fixed this. In the latest nightly this is the url I get.

webSocketDebuggerUrl: "ws://localhost:9999/node"

Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
Fix the `webSocketDebuggerUrl` and `devtoolsFrontendUrl` returned by
v8_inspector in /json HTTP endpoint to work with 3rd party clients.

Fixes: #7227
PR-URL: #7232
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
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

Successfully merging a pull request may close this issue.

4 participants