FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

inspector: no URLs when the debugger is connected by eugeneo · Pull Request #8919 · nodejs/node · GitHub

/ node Public

inspector: no URLs when the debugger is connected - #8919

Closed
eugeneo wants to merge 0 commit into
nodejs:masterfrom
eugeneo:no_urls_when_connected
ConversationCommits (0)ChecksFiles changed
Closed

inspector: no URLs when the debugger is connected#8919
eugeneo wants to merge 0 commit into
nodejs:masterfrom
eugeneo:no_urls_when_connected

Conversation

eugeneo commented Oct 4, 2016

Copy link
Copy Markdown
Contributor
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.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Oct 4, 2016

jbergstroem commented Oct 4, 2016

Copy link
Copy Markdown
Member

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

eugeneo commented Oct 4, 2016

Copy link
Copy Markdown
Contributor Author

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

eugeneo commented Oct 14, 2016

Copy link
Copy Markdown
Contributor Author

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

alexkozy approved these changes Oct 17, 2016
View reviewed changes
Comment thread src/inspector_agent.cc Outdated

alexkozy Oct 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

response.c_str() ?

eugeneo Oct 17, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bnoordhuis requested changes Oct 18, 2016
View reviewed changes
Comment thread src/inspector_agent.cc Outdated

bnoordhuis Oct 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

eugeneo Oct 18, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 :)

Comment thread src/inspector_agent.cc Outdated

bnoordhuis Oct 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

eugeneo Oct 18, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks. Done.

Comment thread src/inspector_agent.cc Outdated

bnoordhuis Oct 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

eugeneo Oct 18, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

bnoordhuis Oct 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

eugeneo Oct 20, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

rvagg force-pushed the master branch 3 times, most recently from 6f558a7 to 0e6750d Compare October 18, 2016 17:23

eugeneo commented Oct 18, 2016

Copy link
Copy Markdown
Contributor Author

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

eugeneo commented Oct 19, 2016

Copy link
Copy Markdown
Contributor Author

Fully green CI: https://ci.nodejs.org/job/node-test-pull-request/4583/ :)

ofrobots commented Oct 20, 2016

Copy link
Copy Markdown
Contributor

@bnoordhuis PTAL when you get a chance.

bnoordhuis commented Oct 20, 2016

Copy link
Copy Markdown
Member

I commented a few hours ago. Did anything change?

eugeneo commented Oct 20, 2016
View reviewed changes

eugeneo left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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/

Comment thread src/inspector_agent.cc Outdated

eugeneo Oct 20, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
inspector: no URLs when the debugger is connected
da184f4 
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 commented Oct 20, 2016

Copy link
Copy Markdown
Contributor Author

Landed as da184f4

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
7450fdc 
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 mentioned this pull request Oct 23, 2016
Closed
2 tasks
jasnell pushed a commit that referenced this pull request Oct 24, 2016
inspector: no URLs when the debugger is connected
a54ec7f 
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 commented Nov 15, 2016

Copy link
Copy Markdown
Contributor

@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 commented Nov 18, 2016

Copy link
Copy Markdown
Contributor Author

@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 commented Nov 18, 2016

Copy link
Copy Markdown
Contributor Author

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

eugeneo commented Nov 18, 2016

Copy link
Copy Markdown
Contributor Author

@thealphanerd - I see no relevant CI failures - https://ci.nodejs.org/job/node-test-commit/6044/

MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
inspector: no URLs when the debugger is connected
8cf7368 
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 commented Nov 18, 2016

Copy link
Copy Markdown
Contributor

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

MylesBorins pushed a commit that referenced this pull request Nov 19, 2016
inspector: no URLs when the debugger is connected
b3f8f89 
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 mentioned this pull request Nov 22, 2016
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
+1 more reviewer
alexkozy alexkozy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL