src: fix json payload from inspector #7232
Conversation
nodejs-github-bot
added
the
c++
|
ci: https://ci.nodejs.org/job/node-test-pull-request/2962/ /cc @nodejs/diagnostics @auchenberg @ofrobots edit: ci failed due to linting error... new lint run https://ci.nodejs.org/job/node-test-linter/2880/ |
MylesBorins
added
the
inspector
| " \"faviconUrl\": \"https://nodejs.org/static/favicon.ico\"," | ||
| " \"id\": \"%d\"," | ||
| " \"title\": \"%s\"," | ||
| " \"type\": \"node\"," | ||
| " \"webSocketDebuggerUrl\": \"ws://%s\"" | ||
| " \"webSocketDebuggerUrl\": \"ws://localhost:%d%s\"" |
There was a problem hiding this comment.
I'm not 100% that this should be localhost... it should potentially be the ip associated with the addr incase there is remote debugging going on. Open to modifying this but wanted to check first
There was a problem hiding this comment.
Nice one!
Indeed, it would be nice to get rid of the hard coded localhost and use the
value from addr()
I'm not sure if it's binding to 0.0.0.0 or the local IP (enables remoting
over the network?)
On Wed, Jun 8, 2016 at 1:16 PM Myles Borins [email protected]
wrote:
In src/inspector_agent.cc
#7232 (comment):" \"faviconUrl\": \"https://nodejs.org/static/favicon.ico\"," " \"id\": \"%d\"," " \"title\": \"%s\"," " \"type\": \"node\","
" \"webSocketDebuggerUrl\": \"ws://%s\""" \"webSocketDebuggerUrl\": \"ws://localhost:%d%s\""I'm not 100% that this should be localhost... it should potentially be the
ip associated with the addr incase there is remote debugging going on. Open
to modifying this but wanted to check first—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/7232/files/aca56db0c4691b6080290792ec8318c04dcb6347#r66331013,
or mute the thread
https://github.com/notifications/unsubscribe/AAKl9ySsaar49QaCCK7P28BUgi8ZlXSJks5qJyMlgaJpZM4IxWgR
.
There was a problem hiding this comment.
I'd leave it as a localhost for now.
There was a problem hiding this comment.
@auchenberg I'll dig into this some more and maybe make a different PR that builds on this one.
MylesBorins
force-pushed
the
fix-inspector-json
branch
from
aca56db
to
e1922b0
Compare
|
PR lgtm as far as v8_inspector is concerned. |
|
LGTM with the v8_inspector part signed off. |
|
LGTM. Thanks for fixing this! |
MylesBorins
force-pushed
the
fix-inspector-json
branch
from
e1922b0
to
853b845
Compare
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 <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>
|
landed in 853b845 |
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 <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Checklist
make -j4 test(UNIX) orvcbuild test nosign(Windows) passesAffected core subsystem(s)
src
Description of change
Fix the
webSocketDebuggerUrlanddevtoolsFrontendUrlreturned byv8_inspector in /json HTTP endpoint to work with 3rd party clients.
Fixes: #7227