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: no URLs when the debugger is connected #8919

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@eugeneo
Contributor

eugeneo commented Oct 4, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

Description of change

By convention, inspector protocol targets do not advertise connection
URLs when the frontend is already connected as multiple inspector
protocol connections are not supported.

@jbergstroem

This comment has been minimized.

Member

jbergstroem commented Oct 4, 2016

(linter fail is my bad -- reimaging one of the vm's)

@eugeneo

This comment has been minimized.

Contributor

eugeneo commented Oct 4, 2016

Windows failures do not seem to be caused by this change.

@eugeneo

This comment has been minimized.

Contributor

eugeneo commented Oct 14, 2016

CI: https://ci.nodejs.org/job/node-test-commit/5605/ - seems to be as green as possible

@ak239

ak239 approved these changes Oct 17, 2016

src/inspector_agent.cc Outdated
inspector_write(socket, header, header_len);
inspector_write(socket, response, len);
inspector_write(socket, &response[0], response.length());

This comment has been minimized.

@ak239

ak239 Oct 17, 2016

Member

response.c_str() ?

This comment has been minimized.

@eugeneo

eugeneo Oct 17, 2016

Contributor

I used data() - there's is no need for null-terminated string.

src/inspector_agent.cc Outdated
}
json << "\n} ]";
return json.str();
}

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 18, 2016

Member

A homegrown JSON serializer seems like a recipe for disaster. Is there a way to use V8's JSON.stringify?

If nothing else, FormatJSON() is something of a misnomer because you need to escape the values manually.

This comment has been minimized.

@eugeneo

eugeneo Oct 18, 2016

Contributor

v8::JSON::stringify requires V8 context. This code runs on a dedicated (non-V8) thread, can be ran early in the startup (e.g. if the node is started with --debug-brk), needs to run when V8 is suspended on a breakpoint, etc.

I renamed to MapToString. I hope we will not need a "full" JSON serializer :)

src/inspector_agent.cc Outdated
inspector_write(socket, header, header_len);
inspector_write(socket, response, len);
inspector_write(socket, response.data(), response.length());

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 18, 2016

Member

This is the tiniest of nits but we use string.size()everywhere else. Can you use it here too?

This comment has been minimized.

@eugeneo

eugeneo Oct 18, 2016

Contributor

Thanks. Done.

src/inspector_agent.cc Outdated
response["User-Agent"] =
"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36"
"(KHTML, like Gecko) Chrome/45.0.2446.0 Safari/537.36";
response["WebKit-Version"] = "537.36 (@198122)";

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 18, 2016

Member

For my education: is this header necessary and if so, what does it signify?

This comment has been minimized.

@eugeneo

eugeneo Oct 18, 2016

Contributor

This is for protocol clients, that are trying to figure out protocol version from this field. It is a wrong check, but apparently some clients did it at the time when the integration was initially implemented.

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 20, 2016

Member

If it's already obsolete baggage, why add it? I assume those clients you mention will need to be updated anyway.

This comment has been minimized.

@eugeneo

eugeneo Oct 20, 2016

Contributor

I agree. Removed the fields. VSCode and Chrome devtools seem unaffected, I do not know what was the tool that used to fail.

@rvagg rvagg force-pushed the nodejs:master branch 3 times, most recently to 0e6750d Oct 18, 2016

@eugeneo

This comment has been minimized.

Contributor

eugeneo commented Oct 18, 2016

@bnoordhuis Thank you for the review. I uploaded a new version of the patch.

@eugeneo

This comment has been minimized.

Contributor

eugeneo commented Oct 19, 2016

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Oct 20, 2016

@bnoordhuis PTAL when you get a chance.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 20, 2016

I commented a few hours ago. Did anything change?

@eugeneo

@bnoordhuis Thank you for the review. I removed those 2 fields and did some manual testing.

New CI: https://ci.nodejs.org/job/node-test-pull-request/4597/

src/inspector_agent.cc Outdated
response["User-Agent"] =
"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36"
"(KHTML, like Gecko) Chrome/45.0.2446.0 Safari/537.36";
response["WebKit-Version"] = "537.36 (@198122)";

This comment has been minimized.

@eugeneo

eugeneo Oct 20, 2016

Contributor

I agree. Removed the fields. VSCode and Chrome devtools seem unaffected, I do not know what was the tool that used to fail.

eugeneo pushed a commit that referenced this pull request Oct 20, 2016

Eugene Ostroukhov
inspector: no URLs when the debugger is connected
By convention, inspector protocol targets do not advertise connection
URLs when the frontend is already connected as multiple inspector
protocol connections are not supported.

PR-URL: #8919
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@eugeneo

This comment has been minimized.

Contributor

eugeneo commented Oct 20, 2016

Landed as da184f4

@eugeneo eugeneo closed this Oct 20, 2016

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Oct 21, 2016

src: fix -Wunused-function warning
Fix the following compile-time warning:

    ../src/inspector_agent.cc:175:19: warning: 'const string
    node::inspector::{anonymous}::to_string(uint64_t)' defined but not
    used [-Wunused-function]
     const std::string to_string(uint64_t number) {

Refs: nodejs#8919

@bnoordhuis bnoordhuis referenced this pull request Oct 23, 2016

Closed

src: remove unused function #9243

2 of 2 tasks complete

jasnell added a commit that referenced this pull request Oct 24, 2016

inspector: no URLs when the debugger is connected
By convention, inspector protocol targets do not advertise connection
URLs when the frontend is already connected as multiple inspector
protocol connections are not supported.

PR-URL: #8919
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Nov 15, 2016

@eugeneo I attempted to backport this to v6.x but it broke the test suite

=== release test-inspector ===
Path: inspector/test-inspector
[err] Debugger listening on port 9229.
[err] Warning: This is an experimental feature and could change at any time.
[err] To start debugging, open the following URL in Chrome:
[err]     chrome-devtools://devtools/remote/serve_file/@60cd6e859b9f557d2312f5bf532f6aec5f284980/inspector.html?experiments=true&v8only=true&ws=localhost:9229/e10ba451-8f65-4866-87ce-ed2d8b7e28fd
[err]
AssertionError: false == true
    at TestSession.session.testHttpResponse (/Users/thealphanerd/code/node/v6.x/test/inspector/test-inspector.js:158:12)
    at checkHttpResponse (/Users/thealphanerd/code/node/v6.x/test/inspector/inspector-helper.js:288:15)
    at IncomingMessage.res.on.on (/Users/thealphanerd/code/node/v6.x/test/inspector/inspector-helper.js:89:24)
    at emitNone (events.js:91:20)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
Command: out/Release/node /Users/thealphanerd/code/node/v6.x/test/inspector/test-inspector.js

thoughts?

@eugeneo

This comment has been minimized.

Contributor

eugeneo commented Nov 18, 2016

@thealphanerd I tried cherrypicking this change on top of v6.x-staging - tests seem to pass - https://github.com/eugeneo/node/tree/v6.x-staging

@eugeneo

This comment has been minimized.

Contributor

eugeneo commented Nov 18, 2016

@thealphanerd - I've also started CI here to see if this is something platform specific.

@eugeneo

This comment has been minimized.

Contributor

eugeneo commented Nov 18, 2016

MylesBorins added a commit that referenced this pull request Nov 18, 2016

inspector: no URLs when the debugger is connected
By convention, inspector protocol targets do not advertise connection
URLs when the frontend is already connected as multiple inspector
protocol connections are not supported.

PR-URL: #8919
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Nov 18, 2016

@eugeneo I've landed it now... must have been something else that landed in the mean time that fixed it
¯_(ツ)_/¯

MylesBorins added a commit that referenced this pull request Nov 19, 2016

inspector: no URLs when the debugger is connected
By convention, inspector protocol targets do not advertise connection
URLs when the frontend is already connected as multiple inspector
protocol connections are not supported.

PR-URL: #8919
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@MylesBorins MylesBorins referenced this pull request Nov 22, 2016

Merged

v6.9.2 proposal #9735

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment