Add v8_inspector support #6792

Closed
wants to merge 5 commits into
from

Projects

None yet
@ofrobots
Contributor
ofrobots commented May 16, 2016 edited

For background see: #2546 (comment)

This PR brings in v8_inspector support to Node.js (currently behind a compile time flag.) This means Node.js can now be debugged and profiled via the Chrome Debugging Protocol, which enables inspection via Chrome DevTools and other tools & IDEs. So far, v8_inspector includes support for the following protocol domains: Debugger, Profiler, Runtime, HeapProfiler

In the Chrome DevTools client, the following is now available:

  • Complete breakpoint debugging, stepping w/ blackboxing
  • Source maps for transpiled code
  • LiveEdit: JavaScript hot-swap evaluation w/ V8
  • Console evaluation with ES6 feature/object support and custom object formatting
  • Sampling JavaScript profiler w/ flamechart
  • Heap snapshot inspection, heap allocation timeline, allocation profiling
  • Asynchronous stacks for native promises

There is more functionality that could be implemented, but we've reached a good point to get input from the community on the feature set and on the integration with Node.js core.

This PR has two logical sets of changes:

Bring in v8_inspector as a dependency

v8_inspector used to be part of Blink. We have removed almost all of the dependencies on Blink and are working on getting this code merged directly into V8 – the biggest piece remaining is integrating the DevTools tests into the V8 test infrastructure. However, this work will likely complete in V8 5.3 / 5.4 timeframe. If we wait for this upstream change, it would mean the improved debugging experience would not be available to Node.js v6.x LTS users.

In the meantime, we propose to bring the v8_inspector dependency directly into Node.js. This dependency can be dropped once v8_inspector merges upstream into V8 (in Node.js v7.x timeframe).

Node.js support for the v8_inspector protocol

The support for talking to v8_inspector protocol is primarily in node_inspector.cc. We also needed support for a subset of the websockets protocol to be able to talk to DevTools. We have implemented this based on the websockets implementation from Chromium. We are calling it the 'inspector socket protocol' as it is not a complete websockets implementation, although it could be extended. This protocol is not available to userspace JavaScript code, and is accessible only to internal C++ code in core.

The implementation is behind a compile time flag controlled by the --with-inspector configure flag.

Usage

$ make -j8
$ ./node --inspect benchmark/http_simple.js
Debugger listening on port 5858. To start debugging, open following URL in Chrome:

chrome-devtools://devtools/remote/serve_file/@4604d24a75168768584760ba56d175507941852f/inspector.html?experiments=true&v8only=true&ws=localhost:5858/node

You can attach Chrome DevTools by opening the printed url in a Chrome browser.

2016-05-16

By default, the debugging port is 5858. You can optionally specify an alternate port via the --inspect=9229 flag. If you want the debugger to pause on application start, you can additional pass the --debug-brk flag. For example:

$ ./node --inspect=9229 --debug-brk benchmark/http_simple.js
Debugger listening on port 9229. To start debugging, open following URL in Chrome:

chrome-devtools://devtools/remote/serve_file/@4604d24a75168768584760ba56d175507941852f/inspector.html?experiments=true&v8only=true&ws=localhost:9229/node

Getting this merged into Node.js would allow people to test the DevTools integration and allow proper debugging to be well-tested and stable well in time for the release of the 6.x LTS. It would also enable third-party debug tool writers to start building other debugging and profiling tools that work with Node.js.

Thanks for the review.

/cc @repenaxa, @eugeneo, @paulirish

@jasnell jasnell commented on the diff May 16, 2016
deps/v8_inspector/README.md
@@ -0,0 +1,2 @@
+# v8_inspector
+# v8_inspector
@jasnell
jasnell May 16, 2016 Member

Useful README is Useful ;-)

@mscdex mscdex commented on an outdated diff May 16, 2016
src/inspector_agent.cc
+// We need pid to use as ID with Chrome
+#if defined(_MSC_VER)
+#include <direct.h>
+#include <io.h>
+#define getpid GetCurrentProcessId
+#else
+#include <unistd.h> // setuid, getuid
+#endif
+
+namespace {
+
+const char DEVTOOLS_PATH[] = "/node";
+
+void PrintDebuggerReadyMessage(int port) {
+ fprintf(stderr, "Debugger listening on port %d. "
+ "To start debugging, open following URL in Chrome:\n"
@mscdex
mscdex May 16, 2016 Contributor

nit: s/open following/open the following/

@mscdex
Contributor
mscdex commented May 16, 2016

Awesome! Thanks everyone that helped put all this together and made it possible!

@mscdex mscdex added the debugger label May 16, 2016
@MylesBorins
Member
MylesBorins commented May 16, 2016 edited

@ofrobots I'm a bit late to the party so sorry if this has been covered in the other thread... Would this PR landing change anything in regards to the current debugger?

@jasnell
Member
jasnell commented May 16, 2016

Great to see this come through. Looking forward to playing around with it.

@ofrobots
Contributor
ofrobots commented May 16, 2016 edited

@TheAlphaNerd The old debugger is not affected, and still works as-is. v8_inspector debugger gets enabled only when the --inspect option is provided.

@cjihrig
Member
cjihrig commented May 16, 2016

My initial reaction is that this is fantastic. Thank you.

@sam-github
Member

@ofrobots

v8_inspector debugger gets enabled only when the --inspect option is provided.

It would be great if it could be enabled/disabled at runtime using a v8.debugOn/Off() js API call.

@trevnorris
Contributor

@ofrobots Why import the entire inspector?

@jasnell
Member
jasnell commented May 16, 2016

Why import the entire inspector?

Was wondering that myself...

@pavelfeldman
Contributor

Why import the entire inspector?

What do you mean by entire inspector?

It would be great if it could be enabled/disabled at runtime using a v8.debugOn/Off() js API call.

Does present debugger allow that? We were aiming at feature parity before we start adding features. Enabling dynamically means that we would bind port at runtime. Other than that, debugger can be enabled/disabled at any moment, no technical limitations there.

@Qard
Member
Qard commented May 16, 2016

Nice work!

So the intent is for this to get merged to v6 and then removed again at v7 when it's baked into V8? Why not just wait until v7 when it's built-in? Certainly it's valuable work and would benefit v6 greatly, but this is a rather large chunk of stuff to review only to remove it again in a few months.

What does the plan look like for merging with V8? I assume the user-facing surface of it would stay the same when we reach v7? (Other than maybe not needing the build flag anymore?)

@jasnell
Member
jasnell commented May 16, 2016

What do you mean by entire inspector?

The second commit -> a7f339b

@ofrobots
Contributor
ofrobots commented May 16, 2016 edited

This PR is not bringing in the DevTools inspector. It only brings in the v8_inspector part, which is an implementation of the DevTools debug protocol.

It would be great if it could be enabled/disabled at runtime using a v8.debugOn/Off() js API call.

This would be a good follow-on. Once this PR lands, it would be easier to work on the ergonomics & integration.

but this is a rather large chunk of stuff to review only to remove it again in a few months.

The review is needed on 0995140. This integrates the inspector protocol into Node-core. This code is is not going to be removed in a few months. Once this is reviewed and merged, it would be an independent question whether this should merge into v6.x. Part of the answer depends on whether this is a semver major change.

The other, larger, commit is a dependency that is needed until the dependency merges upstream into V8. Here's the upstream repository: https://chromium.googlesource.com/chromium/src/third_party/WebKit/Source/platform/v8_inspector/. This commit, like other dependency rolls, doesn't really need an in-depth review.

EDIT: fix link.

@jkrems
Contributor
jkrems commented May 16, 2016 edited

Thanks so much for all this work! Really looking forward to having a "real"/modern debugger protocol for node.

I assume that since the --inspector flag already is disabled by default, it might be possible to enable --with-inspector by default during the lifetime of node 6? Getting a "real" debugger that is part of all the binary distributions of node as part of the next LTS would be great.

The above ("what will become the next LTS") is also, imo, a good reason for bringing it into node 6 and to not wait until all the inspector pieces landed in v8 itself.

@Fishrock123
Member

Hmmm, when we were originally discussing this I was under the impression we'd only have the hooks for the v8 debugger but not actually ship the v8 debugger?

IIRC the v8 (chrome?) debugger is very version agnostic so there wasn't any great need for it to ship in core, and shipping in core is pretty awkward as we see with npm. e.g. you're never quite on the version you want to be. Plus it's a ton of extra source code.

This is assuming the v8/chrome debugger doesn't need native compilation, if it does, that is probably going to be a whole other issue.

Another question: is this intending to replace the default node debugger?

@ofrobots
Contributor
ofrobots commented May 16, 2016 edited

Hmmm, when we were originally discussing this I was under the impression we'd only have the hooks for the v8 debugger but not actually ship the v8 debugger?

This PR does not include the Devtools debugger. It only includes the debugger protocol implementation. When you run with --inspect you see the following on the command line:

chrome-devtools://devtools/remote/serve_file/@4604d24a75168768584760ba56d175507941852f/inspector.html?experiments=true&v8only=true&ws=localhost:5858/node

Chrome will load the version of inspector identified by the id, served from the DevTools web hosts. The only thing Node is doing, is implementing a debug server that 'talks' the protocol expected by the Devtools debugger.

Another question: is this intending to replace the default node debugger?

No.

@Fishrock123
Member

Wow, I'm really surprised that it is this large given that it's just the hooks...

@jasnell
Member
jasnell commented May 16, 2016

Ok, so it's the hooks + the protocol, that makes sense. Will have to spend some time digging into that second commit. The size is a bit concerning but not too bad.

@ofrobots
Contributor

BTW, there are other debuggers that already talk the same protocol, for example: https://github.com/Microsoft/vscode-chrome-debug. It is conceivable that these debuggers could be extended to support Node.js debugging.

@Qard
Member
Qard commented May 16, 2016

Sorry, I was a bit vague. I meant to ask specifically about if the dependency would get removed in v7 (due to being integrated in V8).

Merging 0995140 seems reasonable to me. It's just a7f339b that I'm a bit wary of, since it'd be a bunch of churn to add it and then remove it again. Am I understanding the plan right? The dep get added in v6, then hopefully merged into V8 and removed again for probably v7?

@Qard
Member
Qard commented May 16, 2016

FWIW, I'm leaning more toward just merging it for the sake of making debugging in v6 LTS much nicer. I'm just a bit unsure of the semver-minor-ness of this and the extra work to pull the dep in and maintain it, if we already know we're going to need to remove it again later and get it through the V8 dep.

@jasnell
Member
jasnell commented May 16, 2016

@ofrobots ... are the jinja and markupsafe dependencies really necessary? What are those being used for? (I could keep digging and find out but figured it would be easier to ask ;-) ..)

@MylesBorins
Member

If there is push back on a7f339b due to size, could it be brought in similar to the way we were handling icu before?

@pavelfeldman
Contributor

are the jinja and markupsafe dependencies really necessary?

those are build-time dependencies, we are using jinja to generate native protocol dispatcher and protocol data structures off protocol.json.

@jasnell
Member
jasnell commented May 16, 2016

@TheAlphaNerd ... my concern is not so much about size but of complexity. I think I'm good with this, was just surprised a bit at about how much more code this brings in.

@evanlucas
Member

@repenaxa so I'm not sure how much of protocol.json is browser specific (and I am assuming it is just being pulled in from somewhere else), but how many items in protocol.json are not needed for node? (https://github.com/ofrobots/node/blob/v8-inspector/deps/v8_inspector/devtools/protocol.json#L557-L573)

This is awesome by the way!!! Thanks!

@pavelfeldman
Contributor

how many items in protocol.json are not needed for node?

v8_inspector only supports 4 of 30 domains (Runtime, Debugger, Profiler and HeapProfiler). It is likely to support 2 more later (Console and Tracing). Here is the Blink version of the protocol vs v8_inspector subset.

As we move v8_inspector into v8, we'll have to split the protocol definition and maintain the debugging subset along with v8.

@evanlucas
Member

@repenaxa thanks!

@pavelfeldman
Contributor
pavelfeldman commented May 17, 2016 edited

Quick reference on the file/folder structure of the change:

v8_inspector

devtools/ - remote debugging protocol definition. protocol.json defines the protocol, Inspector-1.1.json contains baseline for backwards compatibility validation. Protocol can be extended with Node-specific domains for non-JS runtime inspection.

platform/inspector_protocol/ - generic remote debugging protocol harness. Generates native API interfaces, data structures and wired dispatcher implementation. Uses deps/jinja2 and deps/markupsafe to emit code at build time.

platform/v8_inspector/ - actual debugger implementation.

All these are single source with the chromium versions, on their way to V8 repo: protocol.json, inspector_protocol, v8_inspector.

node

inspector_agent.* - similarly to debug-agent.*, wires debugger to Node

inspector_socket.* - bare native WebSockets implementation sufficient to power remote debugging

@a7madgamal

awesome! I'm already using node-inspector and can't live without it!
can I know what is the main difference?
consider this comment also as a 'tip' that might help the two projects work together or share knowledge :)

@pavelfeldman
Contributor

I'm already using node-inspector and can't live without it! can I know what is the main difference?

These projects are somewhat related. Both have strong points and should converge later on.

feature set

v8_inspector delivers more fresh features (es6 support, async stacks, integrated profiling, inline values, blackboxing, source maps, workspaces and many more). Its backend is going to be a part of V8 itself. Multiple front-ends including Chrome DevTools, Firefox Tools or Microsoft VS Code will attach to it and debug it (more on that later).

node-inspector has been in the Node space for a long time and captures important debugging ergonomics aspects that are native to Node environment. Things that we missed while developing tooling for the web browsers. We want them supported in the new toolchain as well.

background

Chromium DevTools has client-server architecture where native backend is a part of Blink rendering engine and the front-end is implemented as a web app. There are non-chromium projects that reuse front-end to debug non-web targets (Stetho, node-inspector, etc). There are also non-DevTools clients that attach to the backend for debugging, performance and automation. We call this communication channel remote debugging protocol and it has been supported by Microsoft and Mozilla in their respective clients.

front-end

Both node-inspector and v8_inspector reuse Chrome DevTools front-end. node-inspector rolls the deps periodically (last roll was about a year ago), v8_inspector uses the tip of the tree (the version that matches the v8 release commit).

backend

More importantly, v8_inspector uses the tip of tree native Blink's (soon to be v8's) backend that exposes remote debugging protocol, while node_inspector uses a rudimentary v8-debug protocol that is no longer maintained. node-inspector has a bridge that converts v8-debug terms into remote debugging terms. While working on Web Inspector (origin of DevTools) some time in 2010, we realized the limitations of v8-debug and kicked off the remote debugging protocol. We've been exposing the new features there only since then.

under the hood

v8_inspector brings in the standard infrastructure for exposing runtime capabilities and instrumentation. It enables scaleability via the extensible domain model that Node can use to debug aspects specific to Node or its modules. Here is how Chromium exposes its capabilities via its 30 domains.

@mhdawson
Contributor

What all platforms/os's has this been tested for ?

@ofrobots
Contributor

We have tested the integration on Linux and Mac. Other unices should work too, but they need independent verification. There isn't any architecture specific code that I am aware of, so this should also work on ARM, Power and zArch.

Windows support should be easy to add (once I figure out how to get the Visual Studio environment on my windows box set up to allow me to build Node). Note however that this PR brings in v8_inspector support is behind a compile time flag, so Windows support can be added incrementally.

@joshgav
Member
joshgav commented May 17, 2016 edited

@ofrobots

Windows support should be easy to add (once I figure out

Keep us updated!

/cc @orangemocha

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
@@ -341,6 +342,7 @@
'xcode_settings': {
'GCC_VERSION': 'com.apple.compilers.llvm.clang.1_0',
'CLANG_CXX_LANGUAGE_STANDARD': 'gnu++0x', # -std=gnu++0x
+ 'OTHER_CPLUSPLUSFLAGS' : ['-stdlib=libc++'],
@bnoordhuis
bnoordhuis May 19, 2016 Member

I think linking to libc++ makes it a semver-major change because it affects compiled add-ons.

@eugeneo
eugeneo May 21, 2016 Contributor

Fixed

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
@@ -117,7 +117,7 @@
'tools/msvs/genfiles',
'deps/uv/src/ares',
'<(SHARED_INTERMEDIATE_DIR)', # for node_natives.h
- 'deps/v8' # include/v8_platform.h
+ 'deps/v8', # include/v8_platform.h
@bnoordhuis
bnoordhuis May 19, 2016 Member

Unnecessary change.

@eugeneo
eugeneo May 21, 2016 Contributor

Fixed

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ free(handle);
+}
+
+void DisposeInspector(inspector_socket_t* socket, int status) {
+ free(socket);
+}
+
+void DisconnectAndDisposeIO(inspector_socket_t* socket) {
+ if (socket) {
+ inspector_close(socket, DisposeInspector);
+ }
+}
+
+void OnBufferAlloc(uv_handle_t* handle, size_t len, uv_buf_t* buf) {
+ if (len > 0) {
+ buf->base = reinterpret_cast<char*>(malloc(len));
@bnoordhuis
bnoordhuis May 19, 2016 Member

static_cast

@eugeneo
eugeneo May 21, 2016 Contributor

Fixed

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ inspector_write(socket, header, header_len);
+ inspector_write(socket, response, len);
+}
+
+void SendVersionResponse(inspector_socket_t* socket) {
+ const char VERSION_RESPONSE_TEMPLATE[] =
+ "[ {"
+ " \"Browser\": \"node.js/%s\","
+ " \"Protocol-Version\": \"1.1\","
+ " \"User-Agent\": \"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36"
+ "(KHTML, like Gecko) Chrome/45.0.2446.0 Safari/537.36\","
+ " \"WebKit-Version\": \"537.36 (@198122)\""
+ "} ]";
+ char buffer[sizeof(VERSION_RESPONSE_TEMPLATE) + 128];
+ size_t len = snprintf(buffer, sizeof(buffer),
+ VERSION_RESPONSE_TEMPLATE, NODE_VERSION);
@bnoordhuis
bnoordhuis May 19, 2016 Member

Can you line up the arguments?

@eugeneo
eugeneo May 21, 2016 Contributor

Fixed.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ inspector_write(socket, response, len);
+}
+
+void SendVersionResponse(inspector_socket_t* socket) {
+ const char VERSION_RESPONSE_TEMPLATE[] =
+ "[ {"
+ " \"Browser\": \"node.js/%s\","
+ " \"Protocol-Version\": \"1.1\","
+ " \"User-Agent\": \"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36"
+ "(KHTML, like Gecko) Chrome/45.0.2446.0 Safari/537.36\","
+ " \"WebKit-Version\": \"537.36 (@198122)\""
+ "} ]";
+ char buffer[sizeof(VERSION_RESPONSE_TEMPLATE) + 128];
+ size_t len = snprintf(buffer, sizeof(buffer),
+ VERSION_RESPONSE_TEMPLATE, NODE_VERSION);
+ ASSERT(len < sizeof(buffer));
@bnoordhuis
bnoordhuis May 19, 2016 Member

ASSERT_LT

@eugeneo
eugeneo May 21, 2016 Contributor

Fixed

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ " \"description\": \"node.js instance\","
+ " \"devtoolsFrontendUrl\": "
+ "\"https://chrome-devtools-frontend.appspot.com/serve_file/"
+ "@4604d24a75168768584760ba56d175507941852f/inspector.html\","
+ " \"faviconUrl\": \"https://nodejs.org/static/favicon.ico\","
+ " \"id\": \"%d\","
+ " \"title\": \"%s\","
+ " \"type\": \"node\","
+ " \"webSocketDebuggerUrl\": \"ws://%s\""
+ "} ]";
+ char buffer[sizeof(LIST_RESPONSE_TEMPLATE) + 4096];
+ char title[2048]; // uv_get_process_title trims the title if too long
+ int err = uv_get_process_title(title, sizeof(title));
+ ASSERT_EQ(0, err);
+ size_t len = snprintf(buffer, sizeof(buffer), LIST_RESPONSE_TEMPLATE,
+ getpid(), title, DEVTOOLS_PATH);
@bnoordhuis
bnoordhuis May 19, 2016 Member

It's not very likely but process.title = '... "boom" ...' would break this, wouldn't it?

@eugeneo
eugeneo May 21, 2016 Contributor

Added some basic escaping code (replaces "unsafe" symbols with underscores)

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ " \"devtoolsFrontendUrl\": "
+ "\"https://chrome-devtools-frontend.appspot.com/serve_file/"
+ "@4604d24a75168768584760ba56d175507941852f/inspector.html\","
+ " \"faviconUrl\": \"https://nodejs.org/static/favicon.ico\","
+ " \"id\": \"%d\","
+ " \"title\": \"%s\","
+ " \"type\": \"node\","
+ " \"webSocketDebuggerUrl\": \"ws://%s\""
+ "} ]";
+ char buffer[sizeof(LIST_RESPONSE_TEMPLATE) + 4096];
+ char title[2048]; // uv_get_process_title trims the title if too long
+ int err = uv_get_process_title(title, sizeof(title));
+ ASSERT_EQ(0, err);
+ size_t len = snprintf(buffer, sizeof(buffer), LIST_RESPONSE_TEMPLATE,
+ getpid(), title, DEVTOOLS_PATH);
+ ASSERT(len < sizeof(buffer));
@bnoordhuis
bnoordhuis May 19, 2016 Member

ASSERT_LT

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ ASSERT_EQ(0, err);
+ size_t len = snprintf(buffer, sizeof(buffer), LIST_RESPONSE_TEMPLATE,
+ getpid(), title, DEVTOOLS_PATH);
+ ASSERT(len < sizeof(buffer));
+ SendHttpResponse(socket, buffer, len);
+}
+
+bool RespondToGet(inspector_socket_t* socket, const char* path) {
+ const char PATH[] = "/json";
+ const char PATH_LIST[] = "/json/list";
+ const char PATH_VERSION[] = "/json/version";
+ const char PATH_ACTIVATE[] = "/json/activate/";
+ if (!strncmp(PATH_VERSION, path, sizeof(PATH_VERSION))) {
+ SendVersionResponse(socket);
+ } else if (!strncmp(PATH_LIST, path, sizeof(PATH_LIST))
+ || !strncmp(PATH, path, sizeof(PATH))) {
@bnoordhuis
bnoordhuis May 19, 2016 Member

Style nit: || goes on the previous line.

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ private:
+ Agent* agent_;
+ bool connected_;
+};
+
+class V8NodeInspector : public blink::V8Inspector {
+ public:
+ V8NodeInspector(Agent* agent, node::Environment* env, v8::Platform* platform)
+ : blink::V8Inspector(env->isolate(), env->context()),
+ agent_(agent),
+ isolate_(env->isolate()),
+ platform_(platform),
+ terminated_(false),
+ running_nested_loop_(false) {}
+
+ void runMessageLoopOnPause(int contextGroupId) override {
@bnoordhuis
bnoordhuis May 19, 2016 Member

Style: can you name this context_group_id?

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+}
+
+void Agent::WaitForDisconnect() {
+ inspector_->runMessageLoopOnPause(0);
+}
+
+// static
+void Agent::ThreadCbIO(Agent* agent) {
+ agent->WorkerRunIO();
+}
+
+// static
+void Agent::OnSocketConnectionIO(uv_stream_t* server, int status) {
+ if (status == 0) {
+ inspector_socket_t* socket = reinterpret_cast<inspector_socket_t*>(
+ malloc(sizeof(inspector_socket_t)));
@bnoordhuis
bnoordhuis May 19, 2016 Member

Can you use sizeof(*socket) and static_cast<inspector_socket*>(...)?

@eugeneo
eugeneo May 21, 2016 Contributor

Done.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+void Agent::WaitForDisconnect() {
+ inspector_->runMessageLoopOnPause(0);
+}
+
+// static
+void Agent::ThreadCbIO(Agent* agent) {
+ agent->WorkerRunIO();
+}
+
+// static
+void Agent::OnSocketConnectionIO(uv_stream_t* server, int status) {
+ if (status == 0) {
+ inspector_socket_t* socket = reinterpret_cast<inspector_socket_t*>(
+ malloc(sizeof(inspector_socket_t)));
+ ASSERT_NE(nullptr, socket);
+ memset(socket, 0, sizeof(inspector_socket_t));
@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ if (status == 0) {
+ inspector_socket_t* socket = reinterpret_cast<inspector_socket_t*>(
+ malloc(sizeof(inspector_socket_t)));
+ ASSERT_NE(nullptr, socket);
+ memset(socket, 0, sizeof(inspector_socket_t));
+ socket->data = server->data;
+ if (inspector_accept(server, socket, Agent::OnInspectorHandshakeIO) != 0) {
+ free(socket);
+ }
+ }
+}
+
+// static
+bool Agent::OnInspectorHandshakeIO(inspector_socket_t* socket,
+ enum inspector_handshake_event state,
+ const char* path) {
@bnoordhuis
bnoordhuis May 19, 2016 Member

Style: can you line up the arguments?

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ inspector_socket_t* socket = reinterpret_cast<inspector_socket_t*>(
+ malloc(sizeof(inspector_socket_t)));
+ ASSERT_NE(nullptr, socket);
+ memset(socket, 0, sizeof(inspector_socket_t));
+ socket->data = server->data;
+ if (inspector_accept(server, socket, Agent::OnInspectorHandshakeIO) != 0) {
+ free(socket);
+ }
+ }
+}
+
+// static
+bool Agent::OnInspectorHandshakeIO(inspector_socket_t* socket,
+ enum inspector_handshake_event state,
+ const char* path) {
+ Agent* agent = reinterpret_cast<Agent*>(socket->data);
@bnoordhuis
bnoordhuis May 19, 2016 Member

static_cast

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+bool Agent::OnInspectorHandshakeIO(inspector_socket_t* socket,
+ enum inspector_handshake_event state,
+ const char* path) {
+ Agent* agent = reinterpret_cast<Agent*>(socket->data);
+ switch (state) {
+ case kInspectorHandshakeHttpGet:
+ return RespondToGet(socket, path);
+ case kInspectorHandshakeUpgrading:
+ return AcceptsConnection(socket, path);
+ case kInspectorHandshakeUpgraded:
+ agent->OnInspectorConnectionIO(socket);
+ return true;
+ case kInspectorHandshakeFailed:
+ return false;
+ default:
+ ASSERT(false);
@bnoordhuis
bnoordhuis May 19, 2016 edited Member

UNREACHABLE()

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ agent->OnInspectorConnectionIO(socket);
+ return true;
+ case kInspectorHandshakeFailed:
+ return false;
+ default:
+ ASSERT(false);
+ }
+}
+
+// static
+void Agent::OnRemoteDataIO(uv_stream_t* stream,
+ ssize_t read,
+ const uv_buf_t* b) {
+ inspector_socket_t* socket =
+ reinterpret_cast<inspector_socket_t*>(stream->data);
+ Agent* agent = reinterpret_cast<Agent*>(socket->data);
@bnoordhuis
bnoordhuis May 19, 2016 Member

static_cast (x2 - I'll stop pointing it out from now on.)

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ return false;
+ default:
+ ASSERT(false);
+ }
+}
+
+// static
+void Agent::OnRemoteDataIO(uv_stream_t* stream,
+ ssize_t read,
+ const uv_buf_t* b) {
+ inspector_socket_t* socket =
+ reinterpret_cast<inspector_socket_t*>(stream->data);
+ Agent* agent = reinterpret_cast<Agent*>(socket->data);
+ if (read > 0) {
+ uv_mutex_lock(&agent->queue_lock_);
+ blink::protocol::String16 str(b->base, read - 1);
@bnoordhuis
bnoordhuis May 19, 2016 Member

Why are you omitting the last octet?

@eugeneo
eugeneo May 20, 2016 Contributor

It is an artifact from an older implementation (that had terminating '\0') - I will fix it.

@eugeneo
eugeneo May 21, 2016 Contributor

Fixed in a new patch

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+// static
+void Agent::WriteCbIO(uv_async_t* async) {
+ auto req = reinterpret_cast<AsyncWriteRequest*>(async->data);
+ req->perform();
+ delete req;
+ uv_close(reinterpret_cast<uv_handle_t*>(async), DisposeAsyncCb);
+}
+
+void Agent::WorkerRunIO() {
+ int err;
+ sockaddr_in addr;
+ uv_tcp_t server;
+ uv_tcp_init(&child_loop_, &server);
+ uv_ip4_addr("0.0.0.0", port_, &addr);
+ server.data = this;
+ err = uv_tcp_bind(&server, (const struct sockaddr*)&addr, 0);
@bnoordhuis
bnoordhuis May 19, 2016 Member

No C-style casts, please.

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ int err;
+ sockaddr_in addr;
+ uv_tcp_t server;
+ uv_tcp_init(&child_loop_, &server);
+ uv_ip4_addr("0.0.0.0", port_, &addr);
+ server.data = this;
+ err = uv_tcp_bind(&server, (const struct sockaddr*)&addr, 0);
+ if (err == 0) {
+ err = uv_listen(reinterpret_cast<uv_stream_t*>(&server), 0,
+ OnSocketConnectionIO);
+ }
+ if (err == 0) {
+ PrintDebuggerReadyMessage(port_);
+ } else {
+ fprintf(stderr, "Unable to open devtools socket: %s\n", uv_strerror(err));
+ assert(false);
@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ if (err == 0) {
+ err = uv_listen(reinterpret_cast<uv_stream_t*>(&server), 0,
+ OnSocketConnectionIO);
+ }
+ if (err == 0) {
+ PrintDebuggerReadyMessage(port_);
+ } else {
+ fprintf(stderr, "Unable to open devtools socket: %s\n", uv_strerror(err));
+ assert(false);
+ }
+ if (!wait_) {
+ uv_sem_post(&start_sem_);
+ }
+ uv_run(&child_loop_, UV_RUN_DEFAULT);
+ uv_close(reinterpret_cast<uv_handle_t*>(&server), nullptr);
+ uv_run(&child_loop_, UV_RUN_NOWAIT);
@bnoordhuis
bnoordhuis May 19, 2016 Member

This should probably be UV_RUN_DEFAULT because the server handle is not guaranteed to close immediately. (It does on UNIX platforms but Windows is more complicated.)

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ new SetConnectedTask(this, true));
+}
+
+void Agent::PostMessages() {
+ if (dispatching_messages_)
+ return;
+ dispatching_messages_ = true;
+ std::vector<blink::protocol::String16> messages;
+ uv_mutex_lock(&queue_lock_);
+ messages.swap(message_queue_);
+ uv_mutex_unlock(&queue_lock_);
+
+ for (auto const& message : messages)
+ inspector_->dispatchMessageFromFrontend(message);
+ uv_async_send(&data_written_);
+ dispatching_messages_ = false;
@bnoordhuis
bnoordhuis May 19, 2016 Member

Why is it necessary to toggle dispatching_messages_ in this function? Is it re-entrant?

@eugeneo
eugeneo May 20, 2016 Contributor

V8 may be interrupted while in this loop. Ugly stuff happens..

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ if (connected) {
+ fprintf(stderr, "Debugger attached.\n");
+ inspector_->connectFrontend(new ChannelImpl(this));
+ } else {
+ PrintDebuggerReadyMessage(port_);
+ inspector_->quitMessageLoopOnPause();
+ inspector_->disconnectFrontend();
+ }
+}
+
+void Agent::Write(const String16& message) {
+ uv_async_t* async = reinterpret_cast<uv_async_t*>(malloc(sizeof(uv_async_t)));
+ ASSERT_NE(async, nullptr);
+ uv_async_init(&child_loop_, async, Agent::WriteCbIO);
+ async->data = new AsyncWriteRequest(this, message);
+ ASSERT_EQ(0, uv_async_send(async));
@bnoordhuis
bnoordhuis May 19, 2016 Member

Use CHECK_EQ here.

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ if (connected_ == connected)
+ return;
+
+ connected_ = connected;
+ if (connected) {
+ fprintf(stderr, "Debugger attached.\n");
+ inspector_->connectFrontend(new ChannelImpl(this));
+ } else {
+ PrintDebuggerReadyMessage(port_);
+ inspector_->quitMessageLoopOnPause();
+ inspector_->disconnectFrontend();
+ }
+}
+
+void Agent::Write(const String16& message) {
+ uv_async_t* async = reinterpret_cast<uv_async_t*>(malloc(sizeof(uv_async_t)));
@bnoordhuis
bnoordhuis May 19, 2016 Member

static_cast and sizeof(*async), please.

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ return;
+
+ connected_ = connected;
+ if (connected) {
+ fprintf(stderr, "Debugger attached.\n");
+ inspector_->connectFrontend(new ChannelImpl(this));
+ } else {
+ PrintDebuggerReadyMessage(port_);
+ inspector_->quitMessageLoopOnPause();
+ inspector_->disconnectFrontend();
+ }
+}
+
+void Agent::Write(const String16& message) {
+ uv_async_t* async = reinterpret_cast<uv_async_t*>(malloc(sizeof(uv_async_t)));
+ ASSERT_NE(async, nullptr);
@bnoordhuis
bnoordhuis May 19, 2016 Member

Use CHECK_NE here.

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+
+ connected_ = connected;
+ if (connected) {
+ fprintf(stderr, "Debugger attached.\n");
+ inspector_->connectFrontend(new ChannelImpl(this));
+ } else {
+ PrintDebuggerReadyMessage(port_);
+ inspector_->quitMessageLoopOnPause();
+ inspector_->disconnectFrontend();
+ }
+}
+
+void Agent::Write(const String16& message) {
+ uv_async_t* async = reinterpret_cast<uv_async_t*>(malloc(sizeof(uv_async_t)));
+ ASSERT_NE(async, nullptr);
+ uv_async_init(&child_loop_, async, Agent::WriteCbIO);
@bnoordhuis
bnoordhuis May 19, 2016 Member

Can you check the return value?

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_agent.cc
+ fprintf(stderr, "Debugger attached.\n");
+ inspector_->connectFrontend(new ChannelImpl(this));
+ } else {
+ PrintDebuggerReadyMessage(port_);
+ inspector_->quitMessageLoopOnPause();
+ inspector_->disconnectFrontend();
+ }
+}
+
+void Agent::Write(const String16& message) {
+ uv_async_t* async = reinterpret_cast<uv_async_t*>(malloc(sizeof(uv_async_t)));
+ ASSERT_NE(async, nullptr);
+ uv_async_init(&child_loop_, async, Agent::WriteCbIO);
+ async->data = new AsyncWriteRequest(this, message);
+ ASSERT_EQ(0, uv_async_send(async));
+}
@bnoordhuis
bnoordhuis May 19, 2016 Member

Does this function execute on a different thread than the one that calls uv_run(&child_loop_, UV_RUN_DEFAULT) runs? If it does, the logic is wrong: it's not safe to call uv_async_init() on a different thread, you need to create it upfront on the uv_run thread. Keep in mind that uv_async_send() calls can be coalesced into a single callback event.

@eugeneo
eugeneo May 20, 2016 Contributor

Will fix it in a separate CL: eugeneo#17

@eugeneo
eugeneo May 24, 2016 Contributor

Fixed it by introducing a second queue and using an async created in advance on a proper thread.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
@@ -0,0 +1,762 @@
+#include "inspector_socket.h"
+
+#include "node_internals.h"
+
+#include "openssl/sha.h" // Sha-1 hash
+
+#include <string.h>
+#include <vector>
+
+#define ACCEPT_KEY_LENGTH 28 // SHA1 has is 20 bytes, 28 in base64
@bnoordhuis
bnoordhuis May 19, 2016 Member

Shouldn't that be 27, rounded up?

@eugeneo
eugeneo May 20, 2016 Contributor

It should be even (terminated with '=')

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+#include "inspector_socket.h"
+
+#include "node_internals.h"
+
+#include "openssl/sha.h" // Sha-1 hash
+
+#include <string.h>
+#include <vector>
+
+#define ACCEPT_KEY_LENGTH 28 // SHA1 has is 20 bytes, 28 in base64
+#define BUFFER_GROWTH_CHUNK_SIZE 1024
+
+#define DUMP_READS 0
+#define DUMP_WRITES 0
+
+#define MAX(a, b) ((a) > (b) ? (a) : (b))
@bnoordhuis
bnoordhuis May 19, 2016 Member

This macro doesn't seem to be used.

@eugeneo
eugeneo May 21, 2016 Contributor

Removed

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+enum ws_decode_result {
+ FRAME_OK, FRAME_INCOMPLETE, FRAME_CLOSE, FRAME_ERROR
+};
+
+#if DUMP_READS || DUMP_WRITES
+static void dump_hex(const char* buf, size_t len) {
+ const char* ptr = buf;
+ const char* end = ptr + len;
+ const char* cptr;
+ char c;
+ int i;
+
+ while (ptr < end) {
+ cptr = ptr;
+ for (i = 0; i < 16 && ptr < end; i++) {
+ printf("%2.2X ", *((unsigned char*) ptr++));
@bnoordhuis
bnoordhuis May 19, 2016 Member

Why is the cast necessary?

@eugeneo
eugeneo May 20, 2016 Contributor

Artifact from an older implementation. I've fixed it, thanks for pointing out.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ } else if (data_length <= 0xFFFF) {
+ frame.push_back(kTwoBytePayloadLengthField | mask_key_bit);
+ frame.push_back((data_length & 0xFF00) >> 8);
+ frame.push_back(data_length & 0xFF);
+ } else {
+ frame.push_back(kEightBytePayloadLengthField | mask_key_bit);
+ char extended_payload_length[8];
+ size_t remaining = data_length;
+ // Fill the length into extended_payload_length in the network byte order.
+ for (int i = 0; i < 8; ++i) {
+ extended_payload_length[7 - i] = remaining & 0xFF;
+ remaining >>= 8;
+ }
+ frame.insert(frame.end(), extended_payload_length,
+ extended_payload_length + 8);
+ assert(!remaining);
@bnoordhuis
bnoordhuis May 19, 2016 Member

ASSERT or ASSERT_EQ, please.

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ frame.push_back(kEightBytePayloadLengthField | mask_key_bit);
+ char extended_payload_length[8];
+ size_t remaining = data_length;
+ // Fill the length into extended_payload_length in the network byte order.
+ for (int i = 0; i < 8; ++i) {
+ extended_payload_length[7 - i] = remaining & 0xFF;
+ remaining >>= 8;
+ }
+ frame.insert(frame.end(), extended_payload_length,
+ extended_payload_length + 8);
+ assert(!remaining);
+ }
+
+ if (masking_key != 0) {
+ const char* mask_bytes = reinterpret_cast<char*>(&masking_key);
+ frame.insert(frame.end(), mask_bytes, mask_bytes + 4);
@bnoordhuis
bnoordhuis May 19, 2016 Member

This looks wrong to me, it's endianness-sensitive

@eugeneo
eugeneo May 20, 2016 Contributor

Since this code only has to encode server frames, I'll remove masking support.

@eugeneo
eugeneo May 21, 2016 Contributor

Removed masking.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ size_t remaining = data_length;
+ // Fill the length into extended_payload_length in the network byte order.
+ for (int i = 0; i < 8; ++i) {
+ extended_payload_length[7 - i] = remaining & 0xFF;
+ remaining >>= 8;
+ }
+ frame.insert(frame.end(), extended_payload_length,
+ extended_payload_length + 8);
+ assert(!remaining);
+ }
+
+ if (masking_key != 0) {
+ const char* mask_bytes = reinterpret_cast<char*>(&masking_key);
+ frame.insert(frame.end(), mask_bytes, mask_bytes + 4);
+ for (size_t i = 0; i < data_length; ++i) // Mask the payload.
+ frame.push_back(message[i] ^ mask_bytes[i % kMaskingKeyWidthInBytes]);
@bnoordhuis
bnoordhuis May 19, 2016 Member

So, I infer that endianness is not relevant but this code still looks kind of iffy. If nothing else, replacing kMaskingKeyWidthInBytes with sizeof(masking_key) seems more appropriate; so does using uint32_t instead of int.

@eugeneo
eugeneo May 20, 2016 Contributor

No need for masking code here - we only encode server (e.g. unmasked) frames.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ extended_payload_length + 8);
+ assert(!remaining);
+ }
+
+ if (masking_key != 0) {
+ const char* mask_bytes = reinterpret_cast<char*>(&masking_key);
+ frame.insert(frame.end(), mask_bytes, mask_bytes + 4);
+ for (size_t i = 0; i < data_length; ++i) // Mask the payload.
+ frame.push_back(message[i] ^ mask_bytes[i % kMaskingKeyWidthInBytes]);
+ } else {
+ frame.insert(frame.end(), message, message + data_length);
+ }
+ *output = reinterpret_cast<char*>(malloc(frame.size() + 1));
+ memcpy(*output, &frame[0], frame.size());
+ (*output)[frame.size()] = '\0';
+ *output_len = frame.size();
@bnoordhuis
bnoordhuis May 19, 2016 Member

Why don't you simply write out to a std::vector<char> that the caller passes in by pointer?

@eugeneo
eugeneo May 21, 2016 Contributor

Done.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ default:
+ return FRAME_ERROR;
+ }
+
+ // In Hybi-17 spec client MUST mask its frame.
+ if (client_frame && !masked) {
+ return FRAME_ERROR;
+ }
+
+ uint64_t payload_length64 = second_byte & kPayloadLengthMask;
+ if (payload_length64 > kMaxSingleBytePayloadLength) {
+ int extended_payload_length_size;
+ if (payload_length64 == kTwoBytePayloadLengthField) {
+ extended_payload_length_size = 2;
+ } else {
+ assert(payload_length64 == kEightBytePayloadLengthField);
@bnoordhuis
bnoordhuis May 19, 2016 Member

ASSERT_EQ - although I don't think aborting on malformed external input is appropriate here.

@eugeneo
eugeneo May 21, 2016 Contributor

Error is now returned.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ } else {
+ assert(payload_length64 == kEightBytePayloadLengthField);
+ extended_payload_length_size = 8;
+ }
+ if (buffer_end - p < extended_payload_length_size)
+ return FRAME_INCOMPLETE;
+ payload_length64 = 0;
+ for (int i = 0; i < extended_payload_length_size; ++i) {
+ payload_length64 <<= 8;
+ payload_length64 |= static_cast<unsigned char>(*p++);
+ }
+ }
+
+ size_t actual_masking_key_length = masked ? kMaskingKeyWidthInBytes : 0;
+ static const uint64_t max_payload_length = 0x7FFFFFFFFFFFFFFFull;
+ static size_t max_length = SIZE_MAX;
@bnoordhuis
bnoordhuis May 19, 2016 Member

s/static/static const/? There's no reason for it to be mutable.

@eugeneo
eugeneo May 21, 2016 Contributor

Done

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ payload_length64 <<= 8;
+ payload_length64 |= static_cast<unsigned char>(*p++);
+ }
+ }
+
+ size_t actual_masking_key_length = masked ? kMaskingKeyWidthInBytes : 0;
+ static const uint64_t max_payload_length = 0x7FFFFFFFFFFFFFFFull;
+ static size_t max_length = SIZE_MAX;
+ if (payload_length64 > max_payload_length ||
+ payload_length64 + actual_masking_key_length > max_length) {
+ // WebSocket frame length too large.
+ return FRAME_ERROR;
+ }
+ size_t payload_length = static_cast<size_t>(payload_length64);
+
+ size_t total_length = actual_masking_key_length + payload_length;
@bnoordhuis
bnoordhuis May 19, 2016 Member

This looks wrong, it will silently wrap when sizeof(size_t) <= sizeof(uint32_t) (i.e., 32 bits systems.)

@eugeneo
eugeneo May 21, 2016 Contributor

Changed the code to remove risk of overflow.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ if (payload_length64 > max_payload_length ||
+ payload_length64 + actual_masking_key_length > max_length) {
+ // WebSocket frame length too large.
+ return FRAME_ERROR;
+ }
+ size_t payload_length = static_cast<size_t>(payload_length64);
+
+ size_t total_length = actual_masking_key_length + payload_length;
+ if (static_cast<size_t>(buffer_end - p) < total_length)
+ return FRAME_INCOMPLETE;
+
+ // Add 1 sizeof(char) for 0 terminator
+ *output = reinterpret_cast<char*>(malloc(payload_length + 1));
+ if (masked) {
+ const char* masking_key = p;
+ char* payload = const_cast<char*>(p + kMaskingKeyWidthInBytes);
@bnoordhuis
bnoordhuis May 19, 2016 Member

The const_cast doesn't seem necessary.

@eugeneo
eugeneo May 21, 2016 Contributor

Removed.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ write_to_client(inspector, CLOSE_FRAME, sizeof(CLOSE_FRAME),
+ on_close_frame_written);
+ } else {
+ shutdown_complete(inspector);
+ }
+}
+
+static int parse_ws_frames(inspector_socket_t* inspector, size_t len) {
+ int bytes_consumed = 0;
+ char* output = nullptr;
+ bool compressed = false;
+
+ ws_decode_result r = decode_frame_hybi17(inspector->buffer,
+ len, true /* client_frame */,
+ &bytes_consumed, &output,
+ &compressed);
@bnoordhuis
bnoordhuis May 19, 2016 Member

I suppose I have the same question here: why don't you write the result to a std::vector<char>? That lets you neatly sidestep all the manual memory management and string juggling below.

@eugeneo
eugeneo May 21, 2016 Contributor

Changed to work with vector.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ }
+ free(output);
+ return bytes_consumed;
+}
+
+static void prepare_buffer(uv_handle_t* stream, size_t len, uv_buf_t* buf) {
+ inspector_socket_t* inspector =
+ reinterpret_cast<inspector_socket_t*>(stream->data);
+
+ if (len > (inspector->buffer_size - inspector->data_len)) {
+ int new_size = (inspector->data_len + len + BUFFER_GROWTH_CHUNK_SIZE - 1) /
+ BUFFER_GROWTH_CHUNK_SIZE *
+ BUFFER_GROWTH_CHUNK_SIZE;
+ inspector->buffer_size = new_size;
+ inspector->buffer = reinterpret_cast<char*>(realloc(inspector->buffer,
+ inspector->buffer_size));
@bnoordhuis
bnoordhuis May 19, 2016 Member

Ditto, more or less. Is there a reason you can't turn inspector->buffer into a std::vector<char>?

@eugeneo
eugeneo May 23, 2016 Contributor

This would require some more extensive refactoring, I will address it in a separate commit - eugeneo#18

@bnoordhuis bnoordhuis commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ prepare_buffer,
+ websockets_data_cb);
+ if (err < 0) {
+ close_connection(inspector);
+ }
+ return err;
+}
+
+void inspector_read_stop(inspector_socket_t* inspector) {
+ uv_read_stop(reinterpret_cast<uv_stream_t*>(&inspector->client));
+ inspector->ws_state->alloc_cb = nullptr;
+ inspector->ws_state->read_cb = nullptr;
+}
+
+// From string_bytes.cc
+static size_t base64_encode(const unsigned char* src,
@bnoordhuis
bnoordhuis May 19, 2016 Member

Can you avoid duplicating this function?

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff May 19, 2016
src/inspector_socket.cc
+ }
+ return dlen;
+}
+
+static void generate_accept_string(const char* clientKey, char* buffer) {
+ // Magic string from websockets spec.
+ const char wsMagic[] = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11";
+
+ // sizeof is computed at compile time and accounts for terminating '\0'
+ int len = strlen(clientKey) + sizeof(wsMagic);
+
+ char* buf = reinterpret_cast<char*>(malloc(len));
+ CHECK_NE(buf, nullptr);
+ snprintf(buf, len, "%s%s", clientKey, wsMagic);
+ unsigned char hash[20];
+ SHA1((unsigned char*) buf, len - 1, hash);
@bnoordhuis
bnoordhuis May 19, 2016 Member

Where does this function/macro come from?

@eugeneo
eugeneo May 19, 2016 Contributor

It comes from OpenSSH

@bnoordhuis bnoordhuis commented on an outdated diff May 19, 2016
@@ -3405,6 +3408,17 @@ static bool ParseDebugOpt(const char* arg) {
port = arg + sizeof("--debug-brk=") - 1;
} else if (!strncmp(arg, "--debug-port=", sizeof("--debug-port=") - 1)) {
port = arg + sizeof("--debug-port=") - 1;
+#if HAVE_INSPECTOR
+ // Specifying both --inspect and --debug means debugging is on, using Chromium
+ // inspector.
+ } else if (!strcmp(arg, "--inspect")) {
+ use_debug_agent = true;
+ use_inspector = true;
+ } else if (!strncmp(arg, "--inspect=", sizeof("--inspect=") - 1)) {
+ use_debug_agent = true;
+ use_inspector = true;
+ port = arg + sizeof("--inspect=") - 1;
+#endif
@bnoordhuis
bnoordhuis May 19, 2016 edited Member

Can you make it so that it prints an error message like "V8 inspector support not available with this node.js build"?

EDIT: I mean, in builds with HAVE_INSPECTOR=0.

@ofrobots
Contributor

Thanks for the review @bnoordhuis, we will get the issues addressed. We are also investigating the --stdlib=libc++ requirement on Mac.

In the meanwhile, I have updated the PR with a few fixes. Here's a CI: https://ci.nodejs.org/job/node-test-pull-request/2707/. Note that it is testing the default configuration after this PR (i.e. configure flag --with-inspector is not set.)

@cjihrig
Member
cjihrig commented May 20, 2016

@ofrobots is the plan to keep the --inspector flag? If so, I think we'll need something like cjihrig@5890790 for cluster support.

@ofrobots
Contributor
ofrobots commented May 20, 2016 edited

@cjihrig Yes, the plan to have the --inspect flag in addition to the existing --debug[-brk] flags so that both the old debugger and inspector debug server are available to users. I've added your commit to this PR. Thanks!

@targos
Member
targos commented May 21, 2016

I think there was an idea in #2546 about opening the inspector from chrome://inspect. Is it still planned?

@jkrems
Contributor
jkrems commented May 21, 2016

@targos It looks like it already kind of works. If Chrome is started with --remote-debugging-targets=localhost:5858, the node process appears in chrome://inspect.

@mko-io
mko-io commented May 22, 2016

Can't wait, so good!

@Fishrock123
Member

I'm pretty weary of the maintenance of this in the long term... :/

@ofrobots
Contributor

@Fishrock123 This is comparable in the number of lines of code to the existing debug server in Node that uses the deprecated V8 debug API. The v8_inspector dependency doesn't need maintenance from Node-core; it is an upstream dependency that is en route to get merged into V8. Can you elaborate your exact concern?

@Fishrock123
Member

Right, I'm not worried about the dependency itself, but the inspector.cc and inspector sockets seem like good amount of extra stuff to maintain. You compare it to the debugger, but consider that the debugger is barely maintained and has a good amount of issues related with it.

Also, if this is comparable to the debugger, I'm wondering why we would ship both? They are different in functionality, right?

@eugeneo eugeneo added a commit to eugeneo/node that referenced this pull request May 23, 2016
@eugeneo eugeneo string_bytes: Make base64 encode/decode reusable
Node already has support for base64 encoding and decoding that is not
visible outside the string_bytes.cc file. Our work on providing a
support for the Chrome inspector protocol
(nodejs#6792) requires base64 encoding
support. Rather then introducing a second copy of the base64 encoder,
we suggest moving this code into a separate header.
f327616
@jkrems
Contributor
jkrems commented May 23, 2016

I'd hope that this new debugger protocol can replace the existing debugger completely one day. E.g. it's likely that it would be possible to implement a better command line debugger against the new protocol, given that it has better support for inspecting remote objects etc.. Shipping both enables a graceful transition while a lot of tools still depend on the old protocol.

@ofrobots
Contributor

the debugger is barely maintained and has a good amount of issues related with it.

The addition of the v8_inspector should actually help with this. It would be good to have a debugger written against a proper, documented Debug API as opposed to a long deprecated one.

Destiny of the old debugger is a separate issue and as @jkrems mentions, it makes sense to keep the old debugger for now as it allows for a graceful transition. Let us not conflate these two discussions.

@ofrobots ofrobots added a commit that referenced this pull request May 23, 2016
@eugeneo @ofrobots eugeneo + ofrobots string_bytes: Make base64 encode/decode reusable
Node already has support for base64 encoding and decoding that is not
visible outside the string_bytes.cc file. Our work on providing a
support for the Chrome inspector protocol
(#6792) requires base64 encoding
support. Rather then introducing a second copy of the base64 encoder,
we suggest moving this code into a separate header.

PR-URL: #6910
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
706e699
@yjhjstz
yjhjstz commented May 24, 2016
node-8-inspector  ./node --inspect benchmark/http_simple.js
Debugger listening on port 5858. To start debugging, open following URL in Chrome:
    chrome-devtools://devtools/remote/serve_file/@521e5b7e2b7cc66b4006a8a54cb9c4e57494a5ef/inspector.html?experiments=true&v8only=true&ws=localhost:5858/node
Listening at http://127.0.0.1:8000/
Debugger attached.
Assertion failed: ((0) != (remaining)), function encode_frame_hybi17, file ../src/inspector_socket.cc, line 175.
[1]    40138 abort      ./node --inspect benchmark/http_simple.js

when I take snapshot , got assert error.

@Fishrock123
Member
Fishrock123 commented May 30, 2016 edited

I'm going to skip this in this week's v6 release.

(For the downvoters: You can always build it yourself it you really need it ASAP.)

@ofrobots ofrobots removed the ctc-agenda label May 30, 2016
@ofrobots
Contributor
ofrobots commented May 30, 2016 edited

This was discussed at the ctc meeting last week. The decision at the ctc meeting was to proceed with code reviews. I forgot to remove the label after the meeting.

We have LGTMs on this issue hence the landing.

@dignifiedquire

I'm going to skip this in this week's v6 release.

Could you explain as to why it is not landing in this weeks release, please?

@MylesBorins
Member

@ofrobots was there any decision regarding documentation? I just noticed that this landed without any references to --inspect or how to use the new debugger in the docs.

I'm up for sending a quick PR if the lack of documentation was not intentional and isn't a duplicated effort

@Fishrock123
Member

See my above comments of it landing prematurely, not having correct documentation, etc.

Also we are trying to fix a number of pretty critical bugs right now and I'd prefer to not introduce anything that could slow that further.

@nojvek
nojvek commented May 30, 2016

I just filed an issue regarding the inspect and debug documentation in cli.
I was thinking of sending it as my first node PR in the evening.

Is that the documentation you are talking about?

On Monday, May 30, 2016, Jeremiah Senkpiel notifications@github.com wrote:

See my above comments of it landing prematurely, not having correct
documentation, etc.

Also we are trying to fix a number of pretty critical bugs right now and
I'd prefer to not introduce anything that could slow that further.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6792 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AA-JVIhs1nSggC-ta_hlJZlSvNGIqwzNks5qGxL_gaJpZM4IfrJC
.

@ofrobots
Contributor

@Fishrock123 by CLI options I am assuming that you're asking about the documentation in -h. Given that --debug is not documented either it is incongruous to add --inspect documentation as a prerequisite. See also the comment from @jasnell that this is analogous to AsyncWrap in that both should be considered experimental. Blocking this PR on minor CLI documentation requirements does not seem reasonable. AsyncWrap was landed without any documentation.

Coming up with this PR has been a lot of work for a lot of folks, and continuing to maintain it without landing is not fun.

@Qard
Member
Qard commented May 30, 2016

Documentation suggests some level of stability. I feel like it might be best to leave this undocumented until the inspector is included in V8. Until then, the feature exists but is considered unstable and therefore no promises made on support.

@MylesBorins
Member

@Qard that seems reasonable to me. Perhaps this can be solved with some blogging

@Fishrock123 Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 30, 2016
@eugeneo @Fishrock123 eugeneo + Fishrock123 string_bytes: Make base64 encode/decode reusable
Node already has support for base64 encoding and decoding that is not
visible outside the string_bytes.cc file. Our work on providing a
support for the Chrome inspector protocol
(nodejs#6792) requires base64 encoding
support. Rather then introducing a second copy of the base64 encoder,
we suggest moving this code into a separate header.

PR-URL: nodejs#6910
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
8244d98
@jasnell
Member
jasnell commented May 30, 2016

This landing now seems perfectly appropriate given the ctc discussion, the LGTM's and the experimental stance. There's nothing here that should reasonably interfere with the other bugs that are being looked at and there has been other stuff (e.g. async wrap) that has been landed without complete documentation before. Holding off on including this in a release is perfectly reasonable but I do not see any problem with this landing now.

@cjihrig
Member
cjihrig commented May 30, 2016 edited

Holding off on including this in a release is perfectly reasonable but I do not see any problem with this landing now.

Is there any precedent on keeping legitimate commits, which have already landed, out of releases because of one person's opinions? If the commits aren't going to be reverted, then they should go into the release.

@jasnell
Member
jasnell commented May 30, 2016

If I recall correctly it's happened a few times. Personally, I'm fine with
it either way. It is an experimental feature that's not officially
supported or fully documented yet so there's little harm in it not going
into the next immediate release. However, There's also likely no harm in it
being in the release but if there's someone (particularly a fellow ctc
member) who is unsure then it's worth not rushing.
On May 30, 2016 10:33 AM, "Colin Ihrig" notifications@github.com wrote:

Holding off on including this in a release is perfectly reasonable but I
do not see any problem with this landing now.

Is there any precedent on keeping legitimate commits, which have already
landed, out of releases because of one person's opinions. If the commits
aren't going to be reverted, then they should go into the release.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6792 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eWliYpII-Kwun4dWuLe2w0NkzhPIks5qGx9zgaJpZM4IfrJC
.

@Fishrock123
Member

Is there any precedent on keeping legitimate commits, which have already landed, out of releases because of one person's opinions?

There is a precedent set on doing a patch if the releaser feels that is the better option, but I think there have also been instances of specific commits here and there.

I don't really see what the difference is between an objection before it has landed, or after but before it has been in a release.

(Do not get me wrong, I don't actually object to this in that sort of way.)

@evanlucas
Member

Pulling in commits has more or less been at the releaser's discretion. He had already been planning to make this a semver-patch release since earlier last week though IIRC. I don't see any reason we this can't follow the normal process and just get pulled into the next semver-minor (which will probably follow this week's patch release)

@rvagg
Member
rvagg commented May 30, 2016

Agreed re release decisions, it's up to the releaser to establish their level of comfort with what they are releasing. If something were held back from a couple of consecutive releases even though there's no technical or agreed (by collabs) reason to do so then I think we should be concerned. In this case I agree with @fishrock123, there's some critical stuff we're trying to sort out and it'd be good to try and get stability around those first.

@rvagg
Member
rvagg commented May 30, 2016

@joshgav can we get a perspective from Microsoft and/or @nodejs/tracing regarding this functionality and whether this is a good starting point for a vendor neutral debugging API? Some of us would like some assurance that this isn't tying us even more closely to Chromium.

I know @sidorares has already been messing around with it and seems to have been able to use it for his own tools: https://twitter.com/sidorares/status/736213204410335234

@mscdex
Contributor
mscdex commented May 30, 2016 edited

Is this supposed to work with just --inspect? I'm finding that I have to specify a port explicitly (e.g. --inspect=5959) for it to bind properly (otherwise the process dies with, 'Unable to open devtools socket: address already in use').

.... and now that I try it again with an explicit port, that doesn't seem to work anymore either.... I get the same error message, no matter what the port number is.

@sidorares

@mscdex worked fine for me without port (defaulted to 5858 as expected). Maybe you have script with old debugger agent already running somewhere?

@mscdex
Contributor
mscdex commented May 31, 2016 edited

@sidorares Everytime I check with netstat -nlp, there are no processes listening on whatever port I choose.

EDIT: Ah ok I see now, I have to also add --debug-brk for it to work. The error message is very misleading in that case. If I don't set --debug-brk and I have no debugger; statement, it should just run through without an error right?

@rvagg
Member
rvagg commented May 31, 2016

FYI you can grab a nightly build of master (v7.0.0-nightly...) to try this out, testing would be greatly appreciated before we bring this in to v6: https://nodejs.org/download/nightly/ — right now v7.0.0-nightly20160530894203dae3 is the latest but there might be a new one by the time you're reading this, grab the latest, script the install if you like: https://gist.github.com/rvagg/742f811be491a49ba0b9

@rvagg
Member
rvagg commented May 31, 2016

I've opened #7072 to continue discussion and tied it to the 7.0.0 milestone so we have a decision point regarding continuing to ship this in v6 LTS. If there are outstanding concerns || discussion points I've not mentioned, please add them in there.

@MylesBorins MylesBorins referenced this pull request in nodejs/LTS May 31, 2016
Closed

Track backport of v8_inspector #109

@joshgav
Member
joshgav commented May 31, 2016

@rvagg

@joshgav can we get a perspective from Microsoft and/or @nodejs/tracing regarding this functionality and whether this is a good starting point for a vendor neutral debugging API?

@auchenberg has started updating vscode-chrome-debug-core to support Node+CDP and gain insight, see Microsoft/vscode-chrome-debug-core#17.

Before complete integration and official support, I think we all should understand CDP's stability, compatibility, extensibility, and governance model. That is:

  • Stability - frequency and effect of significant changes;
  • Compatibility - support for various permutations of tools and runtime versions;
  • Extensibility - how to add functionality new to specific tools and runtimes;
  • Governance model - how changes to the protocol are proposed and integrated.

We'll begin discussion in tomorrow's Tracing/Diag WG Meeting.

Will post something similar to #7072. Thanks!

@Chris911
Contributor
Chris911 commented Jun 2, 2016

Reading the latest comments I'm guessing this won't make it to v4? I'm not really familiar with the release life cycle in terms of backporting functionality like this but this is tremendous for debugging and it should be available to as many people as possible. Perhaps when it's no longer an experimental feature?

@rvagg rvagg added a commit that referenced this pull request Jun 2, 2016
@eugeneo @rvagg eugeneo + rvagg string_bytes: Make base64 encode/decode reusable
Node already has support for base64 encoding and decoding that is not
visible outside the string_bytes.cc file. Our work on providing a
support for the Chrome inspector protocol
(#6792) requires base64 encoding
support. Rather then introducing a second copy of the base64 encoder,
we suggest moving this code into a separate header.

PR-URL: #6910
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
756ec80
@auchenberg auchenberg referenced this pull request Jun 6, 2016
Closed

Update inspector ready message to be vendor agnostic #7182

2 of 4 tasks complete
@slikts
slikts commented Jun 8, 2016

Sorry, just to clarify, has this landed in v6? The label says "dont-land-on-v6.x".

@MylesBorins
Member

@slikts that is correct. It will likely land in a future release of v6.x, but we are currently still working out some of the kinks

@develar
develar commented Jun 10, 2016

Thanks to all involved. It is great. No more old slow inconvenient V8 debugger protocol (so, we can get rid of our own v8 debugger protocol extension). After some fixes on the JetBrains IDE side and nodejs (#7248) it works in the IntelliJ IDEs. I haven't yet check sourcemap support but in general it works.

@ofrobots ofrobots deleted the ofrobots:v8-inspector branch Jun 10, 2016
@zcbenz
Contributor
zcbenz commented Jun 14, 2016

@ofrobots Does the v8_inspector shipped in Node have any difference from the one in blink? I would like to bring this to Electron.

@pavelfeldman
Contributor

@zcbenz it should be in sync once @ofrobots lands his PR. Are you going to install it on the isolates that are not serving web contents?

@joshgav
Member
joshgav commented Jun 15, 2016

Started a PR to track support across tools and runtimes here: nodejs/diagnostics#59

@ChALkeR
Member
ChALkeR commented Jun 29, 2016

Is there a reason why this PR forces an openssl build even with --shared-openssl --without-inspector? See #7478.

@ofrobots
Contributor

@ChALkeR I think it is an oversight. I'm looking into it.

@ofrobots ofrobots added a commit to ofrobots/node that referenced this pull request Jun 29, 2016
@ofrobots ofrobots build: make cctest work with --shared-openssl
A new cctest added for v8_inspector was requiring the bundled openssl
unconditionally.

Fixes: nodejs#7478
Ref: nodejs#6792
841b434
@darky darky referenced this pull request in electron/electron Jul 1, 2016
Closed

Electron Async/Promise Error Handling And Stack Traces #6237

@Fishrock123 Fishrock123 added a commit that referenced this pull request Jul 5, 2016
@ofrobots @Fishrock123 ofrobots + Fishrock123 deps: import v8_inspector
Pick up v8 inspector from [1]. This is the standalone version of the
devtools debug protocol.

[1] pavelfeldman/v8_inspector@e1bb206

PR-URL: #6792
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
6210528
@Fishrock123 Fishrock123 added a commit that referenced this pull request Jul 5, 2016
@pavelfeldman @Fishrock123 pavelfeldman + Fishrock123 src,lib: v8-inspector support
This change introduces experimental v8-inspector support. This brings
the DevTools debug protocol allowing Node.js to be debugged with
Chrome DevTools native, or through other debuggers supporting that
protocol.

Partial WebSocket support, to the extent required by DevTools, is
included. This is derived from the implementation in Blink.

v8-inspector support can be disabled by the --without-inspector
configure flag.

PR-URL: #6792
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
7d4f038
@Fishrock123 Fishrock123 added a commit that referenced this pull request Jul 5, 2016
@cjihrig @Fishrock123 cjihrig + Fishrock123 cluster: work with v8_inspector
PR-URL: #6792
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
c1bd3fe
@Fishrock123 Fishrock123 added a commit that referenced this pull request Jul 6, 2016
@Fishrock123 Fishrock123 2016-07-06, Version 6.3.0 (Current)
Notable changes:

* buffer: Added `buffer.swap64()` to compliment `swap16()` &
`swap32()`. (Zach Bjornson) #7157
* build: New `configure` options have been added for building Node.js
as a shared library. (Stefan Budeanu)
#6994
  - The options are: `--shared`, `--without-v8-platform` &
`--without-bundled-v8`.
* crypto: Root certificates have been updated. (Ben Noordhuis)
#7363
* debugger: The server address is now configurable via
`--debug=<address>:<port>`. (Ben Noordhuis)
#3316
* npm: Upgraded npm to v3.10.3 (Kat Marchán)
#7515 & (Rebecca Turner)
#7410
* readline: Added the `prompt` option to the readline constructor.
(Evan Lucas) #7125
* repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops
without stopping the Node.js instance. (Anna Henningsen)
#6635
* src:
  - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao)
#3098
  - Refactored `require('constants')`, constants are now available
directly from their respective modules. (James M Snell)
#6534
* stream: Improved `readable.read()` performance by up to 70%. (Brian
White) #7077
* timers: `setImmediate()` is now up to 150% faster in some situations.
(Andras) #6436
* util: Added a `breakLength` option to `util.inspect()` to control how
objects are formatted across lines. (cjihrig)
#7499
* v8-inspector: Experimental support has been added for debugging
Node.js over the inspector protocol. (Ali Ijaz Sheikh)
#6792
  - *Note: This feature is experimental, and it could be altered or
removed.*
  - You can try this feature by running Node.js with the `--inspect`
flag.

Refs: #7441
PR-URL: #7550
50aede5
@Fishrock123 Fishrock123 added a commit that referenced this pull request Jul 6, 2016
@Fishrock123 Fishrock123 2016-07-06, Version 6.3.0 (Current)
Notable changes:

* buffer: Added `buffer.swap64()` to compliment `swap16()` &
`swap32()`. (Zach Bjornson) #7157
* build: New `configure` options have been added for building Node.js
as a shared library. (Stefan Budeanu)
#6994
  - The options are: `--shared`, `--without-v8-platform` &
`--without-bundled-v8`.
* crypto: Root certificates have been updated. (Ben Noordhuis)
#7363
* debugger: The server address is now configurable via
`--debug=<address>:<port>`. (Ben Noordhuis)
#3316
* npm: Upgraded npm to v3.10.3 (Kat Marchán)
#7515 & (Rebecca Turner)
#7410
* readline: Added the `prompt` option to the readline constructor.
(Evan Lucas) #7125
* repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops
without stopping the Node.js instance. (Anna Henningsen)
#6635
* src:
  - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao)
#3098
  - Refactored `require('constants')`, constants are now available
directly from their respective modules. (James M Snell)
#6534
* stream: Improved `readable.read()` performance by up to 70%. (Brian
White) #7077
* timers: `setImmediate()` is now up to 150% faster in some situations.
(Andras) #6436
* util: Added a `breakLength` option to `util.inspect()` to control how
objects are formatted across lines. (cjihrig)
#7499
* v8-inspector: Experimental support has been added for debugging
Node.js over the inspector protocol. (Ali Ijaz Sheikh)
#6792
  - *Note: This feature is experimental, and it could be altered or
removed.*
  - You can try this feature by running Node.js with the `--inspect`
flag.

Refs: #7441
PR-URL: #7550
9fc01b1
@Fishrock123 Fishrock123 added a commit that referenced this pull request Jul 6, 2016
@Fishrock123 Fishrock123 2016-07-06, Version 6.3.0 (Current)
Notable changes:

* buffer: Added `buffer.swap64()` to compliment `swap16()` &
`swap32()`. (Zach Bjornson) #7157
* build: New `configure` options have been added for building Node.js
as a shared library. (Stefan Budeanu)
#6994
  - The options are: `--shared`, `--without-v8-platform` &
`--without-bundled-v8`.
* crypto: Root certificates have been updated. (Ben Noordhuis)
#7363
* debugger: The server address is now configurable via
`--debug=<address>:<port>`. (Ben Noordhuis)
#3316
* npm: Upgraded npm to v3.10.3 (Kat Marchán)
#7515 & (Rebecca Turner)
#7410
* readline: Added the `prompt` option to the readline constructor.
(Evan Lucas) #7125
* repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops
without stopping the Node.js instance. (Anna Henningsen)
#6635
* src:
  - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao)
#3098
  - Refactored `require('constants')`, constants are now available
directly from their respective modules. (James M Snell)
#6534
* stream: Improved `readable.read()` performance by up to 70%. (Brian
White) #7077
* timers: `setImmediate()` is now up to 150% faster in some situations.
(Andras) #6436
* util: Added a `breakLength` option to `util.inspect()` to control how
objects are formatted across lines. (cjihrig)
#7499
* v8-inspector: Experimental support has been added for debugging
Node.js over the inspector protocol. (Ali Ijaz Sheikh)
#6792
  - *Note: This feature is experimental, and it could be altered or
removed.*
  - You can try this feature by running Node.js with the `--inspect`
flag.

Refs: #7441
PR-URL: #7550
7628031
@avesus
avesus commented Jul 6, 2016

So what about source maps / workspaces support? Chrome said ''Workspace mapping mismatch" because of the wrapper code.

@pavelfeldman
Contributor

Chrome said ''Workspace mapping mismatch" because of the wrapper code.

This is fixed upstream, we need to roll v8_inspector to pick it up.

@lukesampson lukesampson added a commit to lukesampson/scoop that referenced this pull request Jul 7, 2016
@chrjean @lukesampson chrjean + lukesampson Update nodejs to 6.3.0 (#948)
### Notable changes

* **buffer**: Added `buffer.swap64()` to compliment `swap16()` & `swap32()`. (Zach Bjornson) [#7157](nodejs/node#7157)
* **build**: New `configure` options have been added for building Node.js as a shared library. (Stefan Budeanu) [#6994](nodejs/node#6994)
  - The options are: `--shared`, `--without-v8-platform` & `--without-bundled-v8`.
* **crypto**: Root certificates have been updated. (Ben Noordhuis) [#7363](nodejs/node#7363)
* **debugger**: The server address is now configurable via `--debug=<address>:<port>`. (Ben Noordhuis) [#3316](nodejs/node#3316)
* **npm**: Upgraded npm to v3.10.3 (Kat Marchán) [#7515](nodejs/node#7515) & (Rebecca Turner) [#7410](nodejs/node#7410)
* **readline**: Added the `prompt` option to the readline constructor. (Evan Lucas) [#7125](nodejs/node#7125)
* **repl / vm**: `sigint`/`ctrl+c` will now break out of infinite loops without stopping the Node.js instance. (Anna Henningsen) [#6635](nodejs/node#6635)
* **src**:
  - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao) [#3098](nodejs/node#3098)
  - Refactored `require('constants')`, constants are now available directly from their respective modules. (James M Snell) [#6534](nodejs/node#6534)
* **stream**: Improved `readable.read()` performance by up to 70%. (Brian White) [#7077](nodejs/node#7077)
* **timers**: `setImmediate()` is now up to 150% faster in some situations. (Andras) [#6436](nodejs/node#6436)
* **util**: Added a `breakLength` option to `util.inspect()` to control how objects are formatted across lines. (cjihrig) [#7499](nodejs/node#7499)
* **v8-inspector**: Experimental support has been added for debugging Node.js over the inspector protocol. (Ali Ijaz Sheikh) [#6792](nodejs/node#6792)
  - **Note: This feature is _experimental_, and it could be altered or removed.**
  - You can try this feature by running Node.js with the `--inspect` flag.
3b8b000
@rogovdm
rogovdm commented Jul 11, 2016

node 6.3.0

node server --inspect

does not work :|

@sidorares

@rogovdm you are passing argument to script, do node --inspect server instead

@shimaore shimaore added a commit to shimaore/test-pouchdb-memleak that referenced this pull request Oct 19, 2016
@shimaore shimaore remove heapdump
I'll be using --inspect from nodejs/node#6792
af53bd6
@leofab86
leofab86 commented Nov 17, 2016 edited

I cant print to chrome console on Windows. Macbook works fine. Anyone else having this issue?

@uMaxmaxmaximus

how to autostart chrome devtool when i write node --inspect app.js ?

@uMaxmaxmaximus
uMaxmaxmaximus commented Nov 18, 2016 edited

@FredyC

Extention throw error:

Could not launch debugger
Invalid devtools URL in
[ { "description": "node.js instance", "devtoolsFrontendUrl": "chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/74dec2d8-aca4-4725-85af-f292cb39ad13", "faviconUrl": "https://nodejs.org/static/favicon.ico", "id": "74dec2d8-aca4-4725-85af-f292cb39ad13", "title": "./build/server", "type": "node", "url": "file://C:_localhost_taxi_build_server", "webSocketDebuggerUrl": "ws://127.0.0.1:9229/74dec2d8-aca4-4725-85af-f292cb39ad13" } ]
@FredyC
FredyC commented Nov 18, 2016

@uMaxmaxmaximus Sorry, I am not the author of that extension and I don't think this is a right place to try to solve the issue ... https://github.com/continuationlabs/node-v8-inspector

@uMaxmaxmaximus
uMaxmaxmaximus commented Nov 18, 2016 edited

@FredyC
I've tried it, it does not work unfortunately. Maybe there is some way to intercept the link of std.output using node.js? Then I would have written a module that automatically opens a browser to this link.

Maybe link active debugger put to global.__inspectorUrl__ ?
Or create debugger object like this, to control debug from runtime script:

global.debug = {
  url: 'bla bla',
  port: 'bla bla',
  next: function(){ [native code] },
  stepInto: function(){ [native code] },
  e.t.c
}
@FredyC
FredyC commented Nov 18, 2016

@uMaxmaxmaximus I don't know, it works for me (on Windows as well). There is another alternative you might try ... https://github.com/jaridmargolin/inspect-process

@june07
june07 commented Dec 20, 2016

@uMaxmaxmaximus http://june07.com/nim will manage DevTools (including auto launch).

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