src: add /json/protocol endpoint to inspector #7491

bnoordhuis · 2016-06-29T23:24:41Z

Embed the compressed and minified protocol.json from the bundled
v8_inspector and make it available through the /json/protocol endpoint.

Refs: nodejs/diagnostics#52
CI: https://ci.nodejs.org/job/node-test-pull-request/3133/
EDIT: https://ci.nodejs.org/job/node-test-pull-request/3135/

addaleax · 2016-06-29T23:35:57Z

src/inspector_agent.cc

+  strm.opaque = Z_NULL;
+  CHECK_EQ(Z_OK, inflateInit(&strm));
+  static const size_t kDecompressedSize =
+      PROTOCOL_JSON[0] * 0x1000u +


0x10000u?

Indeed. I'll fix that.

jasnell · 2016-06-30T00:05:08Z

LGTM but can you add a quick statement to the commit log about why? :-)

addaleax · 2016-06-30T00:13:50Z

test/testpy/__init__.py

-      result += flags_match.group(1).strip().split()
+      # PORT should match the definition in test/common.js.
+      env = { 'PORT': int(os.getenv('NODE_COMMON_PORT', '12346')) }
+      env['PORT'] += self.thread_id * 100


Adding thread_id * 100 to the port number and requiring the port to match common.PORT seem to be kind of contradictory?

I didn't make it up, exports.PORT += process.env.TEST_THREAD_ID * 100; is what test/common.js does.

Oh, sorry, didn’t see that value is changed later in test/common.js.

addaleax · 2016-06-30T00:25:26Z

This CI failure looks related

joshgav · 2016-06-30T02:34:03Z

Does this commit intend to canonicalize V8's embedded protocol.json as Node's protocol.json? I don't think we should finalize that yet, that's the goal of the discussion in nodejs/diagnostics#52.

bnoordhuis · 2016-06-30T07:33:25Z

This CI failure looks related

Looks like I got the dependency chain wrong. It also looks like we're doing that wrong in more places. I added a fix-up that makes the inspector build depend explicitly on the generated header.

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

Does this commit intend to canonicalize V8's embedded protocol.json as Node's protocol.json?

No.

saghul · 2016-06-30T08:35:57Z

tools/compress_json.py

+import zlib
+
+if __name__ == '__main__':
+  fp = open(sys.argv[1])


not sure if important for this kind of script, but fp is not explicitly closed. Since we depend on 2.6-2.7, maybe use a with statement instead?

The script is short-lived so I went for simplicity.

addaleax · 2016-06-30T08:44:55Z

Apparently, that did not do the trick… https://ci.nodejs.org/job/node-test-commit-arm/3865/nodes=armv7-wheezy/console

bnoordhuis · 2016-06-30T08:45:09Z

I'm starting to think the bug might not be in my changes to node.gyp. Other buildbots do this:

  LD_LIBRARY_PATH=/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/lib.host:/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj/gen; python tools/js2c.py "/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj/gen/node_natives.h" lib/internal/bootstrap_node.js lib/_debug_agent.js lib/_debugger.js lib/assert.js lib/buffer.js lib/child_process.js lib/console.js lib/constants.js lib/crypto.js lib/cluster.js lib/dgram.js lib/dns.js lib/domain.js lib/events.js lib/fs.js lib/http.js lib/_http_agent.js lib/_http_client.js lib/_http_common.js lib/_http_incoming.js lib/_http_outgoing.js lib/_http_server.js lib/https.js lib/_linklist.js lib/module.js lib/net.js lib/os.js lib/path.js lib/process.js lib/punycode.js lib/querystring.js lib/readline.js lib/repl.js lib/stream.js lib/_stream_readable.js lib/_stream_writable.js lib/_stream_duplex.js lib/_stream_transform.js lib/_stream_passthrough.js lib/_stream_wrap.js lib/string_decoder.js lib/sys.js lib/timers.js lib/tls.js lib/_tls_common.js lib/_tls_legacy.js lib/_tls_wrap.js lib/tty.js lib/url.js lib/util.js lib/v8.js lib/vm.js lib/zlib.js lib/internal/child_process.js lib/internal/cluster.js lib/internal/freelist.js lib/internal/linkedlist.js lib/internal/net.js lib/internal/module.js lib/internal/process/next_tick.js lib/internal/process/promises.js lib/internal/process/stdio.js lib/internal/process/warning.js lib/internal/process.js lib/internal/readline.js lib/internal/repl.js lib/internal/socket_list.js lib/internal/util.js lib/internal/v8_prof_polyfill.js lib/internal/v8_prof_processor.js lib/internal/streams/lazy_transform.js lib/internal/streams/BufferList.js deps/v8/tools/splaytree.js deps/v8/tools/codemap.js deps/v8/tools/consarray.js deps/v8/tools/csvparser.js deps/v8/tools/profile.js deps/v8/tools/profile_view.js deps/v8/tools/logreader.js deps/v8/tools/tickprocessor.js deps/v8/tools/SourceMap.js deps/v8/tools/tickprocessor-driver.js ./config.gypi src/notrace_macros.py src/nolttng_macros.py src/perfctr_macros.py
  touch /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj.target/node_dtrace_header.stamp
  touch /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj.target/node_dtrace_provider.stamp
  touch /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj.target/node_dtrace_ustack.stamp
  touch /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj.target/node_etw.stamp
  touch /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj.target/node_perfctr.stamp
  touch /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj.target/specialize_node_d.stamp
  LD_LIBRARY_PATH=/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/lib.host:/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj/gen; python tools/compress_json.py deps/v8_inspector/platform/v8_inspector/js_protocol.json "/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-ubuntu1404/out/Release/obj/gen/v8_inspector_protocol_json.h"

But the armv7-wheezy buildbot only does this:

  LD_LIBRARY_PATH=/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-wheezy/out/Release/lib.host:/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-wheezy/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-wheezy/out/Release/obj/gen; python tools/js2c.py "/home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-wheezy/out/Release/obj/gen/node_natives.h" lib/internal/bootstrap_node.js lib/_debug_agent.js lib/_debugger.js lib/assert.js lib/buffer.js lib/child_process.js lib/console.js lib/constants.js lib/crypto.js lib/cluster.js lib/dgram.js lib/dns.js lib/domain.js lib/events.js lib/fs.js lib/http.js lib/_http_agent.js lib/_http_client.js lib/_http_common.js lib/_http_incoming.js lib/_http_outgoing.js lib/_http_server.js lib/https.js lib/_linklist.js lib/module.js lib/net.js lib/os.js lib/path.js lib/process.js lib/punycode.js lib/querystring.js lib/readline.js lib/repl.js lib/stream.js lib/_stream_readable.js lib/_stream_writable.js lib/_stream_duplex.js lib/_stream_transform.js lib/_stream_passthrough.js lib/_stream_wrap.js lib/string_decoder.js lib/sys.js lib/timers.js lib/tls.js lib/_tls_common.js lib/_tls_legacy.js lib/_tls_wrap.js lib/tty.js lib/url.js lib/util.js lib/v8.js lib/vm.js lib/zlib.js lib/internal/child_process.js lib/internal/cluster.js lib/internal/freelist.js lib/internal/linkedlist.js lib/internal/net.js lib/internal/module.js lib/internal/process/next_tick.js lib/internal/process/promises.js lib/internal/process/stdio.js lib/internal/process/warning.js lib/internal/process.js lib/internal/readline.js lib/internal/repl.js lib/internal/socket_list.js lib/internal/util.js lib/internal/v8_prof_polyfill.js lib/internal/v8_prof_processor.js lib/internal/streams/lazy_transform.js lib/internal/streams/BufferList.js deps/v8/tools/splaytree.js deps/v8/tools/codemap.js deps/v8/tools/consarray.js deps/v8/tools/csvparser.js deps/v8/tools/profile.js deps/v8/tools/profile_view.js deps/v8/tools/logreader.js deps/v8/tools/tickprocessor.js deps/v8/tools/SourceMap.js deps/v8/tools/tickprocessor-driver.js ./config.gypi src/notrace_macros.py src/nolttng_macros.py src/perfctr_macros.py
  touch /home/iojs/build/workspace/node-test-commit-arm/nodes/armv7-wheezy/out/Release/obj.host/node_js2c.stamp

The output of ./configure on the two buildbots isn't hugely different. arm_fpu and arm_version are only used by V8.

--- a.txt       2016-06-30 10:39:49.475993197 +0200
+++ b.txt       2016-06-30 10:40:04.387993197 +0200
@@ -1,7 +1,7 @@
   'variables': { 'arm_float_abi': 'hard',
-                 'arm_fpu': 'vfpv3',
+                 'arm_fpu': 'vfp',
                  'arm_thumb': 0,
-                 'arm_version': '7',
+                 'arm_version': '6',
                  'asan': 0,
                  'debug_devtools': 'node',
                  'gas_version': '2.24',

cc @nodejs/build - ideas on what might be causing that?

EDIT: To be clear, CI runs for other pull requests show the exact same behavior. It's not isolated to this pull request.

bnoordhuis · 2016-07-06T14:34:35Z

Ping @nodejs/build. Even just a 'no clue' would be appreciated. :-)

pavelfeldman · 2016-07-06T19:27:45Z

It might make sense to expose this via the protocol itself i.e. Protocol.getJSON method would return the protocol definition. That way clients could reuse same web socket connection and hence save on CORS, etc. for the separate request.

joaocgreis · 2016-07-07T11:07:34Z

On my local machine this compiles with make -j10 but fails with make -j1. make -n shows that making v8_inspector_protocol_json.h is one of the last things that make tries to do, after compiling inspector_agent.cc. What about adding 'v8_inspector_compress_protocol_json' to the dependencies in line 306 instead of the last fixup?

bnoordhuis · 2016-07-09T12:02:21Z

@joaocgreis Good suggestion, updated. CI: https://ci.nodejs.org/job/node-test-pull-request/3227/

It might make sense to expose this via the protocol itself i.e. Protocol.getJSON method would return the protocol definition. That way clients could reuse same web socket connection and hence save on CORS, etc. for the separate request.

@pavelfeldman Correct me if I'm wrong but doesn't that introduce a chicken-and-egg problem? The client won't know what protocol to speak to request the protocol JSON.

pavelfeldman · 2016-07-11T21:03:09Z

Correct me if I'm wrong but doesn't that introduce a chicken-and-egg problem?

It does, but it is manageable. There could be runtimes with different sets of capabilities available via the same endpoint. If we do use json/protocol to expose those capabilities, then it should be on the connection level in a form of RTTI. We have not implemented these capabilities yet, so it is up for debate.

jasnell · 2016-08-09T21:56:24Z

ping @bnoordhuis :-)

bnoordhuis · 2016-08-10T13:07:48Z

Let's take the debian approach: when faced with several options, do 'em all. Does publishing the protocol over both WS and HTTP sound good?

pavelfeldman · 2016-08-10T15:36:25Z

@bnoordhuis we can do /json/protocol first to cover the immediate needs. we'll follow up with the same in chromium for consistency.

bnoordhuis · 2016-08-15T17:33:04Z

Rebased, new CI: https://ci.nodejs.org/job/node-test-pull-request/3676/

Can I get a LGTM or two?

MylesBorins · 2016-08-15T17:40:54Z

node.gyp

@@ -322,6 +322,7 @@
          'dependencies': [
            'deps/v8_inspector/third_party/v8_inspector/platform/'
                'v8_inspector/v8_inspector.gyp:v8_inspector_stl',
+            'v8_inspector_compress_protocol_json#host',


nit: is there a reason for the inconsistent indent between this and the two lines above?

The previous line is a continuation of the one before it. Note how the first line doesn't have a comma at the end.

bnoordhuis · 2016-08-23T19:06:16Z

Rebased against master. Can someone sign off on this?

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

jasnell · 2016-08-23T19:08:45Z

LGTM

cjihrig · 2016-08-23T23:59:53Z

LGTM

bnoordhuis · 2016-09-11T11:06:21Z

Rebased. New CI run: https://ci.nodejs.org/job/node-test-pull-request/4008/

Barring nays or CI failures, I'll merge this tomorrow or the day after.

EDIT: Again, because "stderr: fatal: unable to access 'https://github.com/nodejs/node.git/': gnutls_handshake() failed: A TLS packet with unexpected length was received."

https://ci.nodejs.org/job/node-test-pull-request/4009/

joshgav · 2016-09-15T22:46:10Z

CI again so we can get this merged: https://ci.nodejs.org/job/node-test-pull-request/4064

EDIT: Problems on ARM and AIX which are out of my realm to troubleshoot 😢

jasnell · 2016-09-20T16:48:51Z

Ping @bnoordhuis

bnoordhuis · 2016-09-23T16:11:30Z

Rebased. CI again: https://ci.nodejs.org/job/node-test-pull-request/4240/

bnoordhuis · 2016-09-23T16:51:27Z

Known flake parallel/test-tick-processor-unknown on smartos, see #8725; everything else is green/yellow. I'm going to land this.

Embed the compressed and minified protocol.json from the bundled v8_inspector and make it available through the /json/protocol endpoint. Refs: nodejs/diagnostics#52 PR-URL: nodejs#7491 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Embed the compressed and minified protocol.json from the bundled v8_inspector and make it available through the /json/protocol endpoint. Refs: nodejs/diagnostics#52 PR-URL: #7491 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

According to nodejs/node#7491

bnoordhuis 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 Jun 29, 2016

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Jun 29, 2016

bnoordhuis mentioned this pull request Jun 29, 2016

Subset of CDP as profile for Node.js tools and VMs nodejs/diagnostics#52

Closed

1 task

addaleax reviewed Jun 29, 2016
View reviewed changes

addaleax reviewed Jun 30, 2016
View reviewed changes

saghul reviewed Jun 30, 2016
View reviewed changes

bnoordhuis force-pushed the diag52 branch from 0fec31a to 4a92a61 Compare July 9, 2016 11:59

xbolshe mentioned this pull request Jul 17, 2016

arm and LD_LIBRARY_PATH (ld-linux-armhf.so.3: No such file or directory) #7770

Closed

bnoordhuis force-pushed the diag52 branch from 4a92a61 to 25e30b6 Compare August 15, 2016 17:31

MylesBorins reviewed Aug 15, 2016
View reviewed changes

bnoordhuis force-pushed the diag52 branch 2 times, most recently from 68206a9 to 2655fb9 Compare August 23, 2016 19:05

bnoordhuis force-pushed the diag52 branch from 2655fb9 to fe76066 Compare September 11, 2016 11:03

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

bnoordhuis force-pushed the diag52 branch from fe76066 to 0e54827 Compare September 23, 2016 16:10

bnoordhuis force-pushed the diag52 branch from 0e54827 to 782620f Compare September 23, 2016 16:54

bnoordhuis closed this Sep 23, 2016

bnoordhuis deleted the diag52 branch September 23, 2016 16:54

bnoordhuis merged commit 782620f into nodejs:master Sep 23, 2016

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

cyrus-and added a commit to cyrus-and/chrome-remote-interface that referenced this pull request Oct 5, 2016

Add the Node.js protocol fetcher

1444a46

According to nodejs/node#7491

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add /json/protocol endpoint to inspector #7491

src: add /json/protocol endpoint to inspector #7491

bnoordhuis commented Jun 29, 2016 •

edited

Loading

addaleax Jun 29, 2016

bnoordhuis Jun 29, 2016

jasnell commented Jun 30, 2016

addaleax Jun 30, 2016

bnoordhuis Jun 30, 2016

addaleax Jun 30, 2016

addaleax commented Jun 30, 2016

joshgav commented Jun 30, 2016

bnoordhuis commented Jun 30, 2016

saghul Jun 30, 2016

bnoordhuis Jun 30, 2016

addaleax commented Jun 30, 2016

bnoordhuis commented Jun 30, 2016 •

edited

Loading

bnoordhuis commented Jul 6, 2016

pavelfeldman commented Jul 6, 2016

joaocgreis commented Jul 7, 2016

bnoordhuis commented Jul 9, 2016

pavelfeldman commented Jul 11, 2016

jasnell commented Aug 9, 2016

bnoordhuis commented Aug 10, 2016

pavelfeldman commented Aug 10, 2016

bnoordhuis commented Aug 15, 2016

MylesBorins Aug 15, 2016

bnoordhuis Aug 15, 2016

bnoordhuis commented Aug 23, 2016

jasnell commented Aug 23, 2016

cjihrig commented Aug 23, 2016

bnoordhuis commented Sep 11, 2016 •

edited

Loading

joshgav commented Sep 15, 2016 •

edited

Loading

jasnell commented Sep 20, 2016

bnoordhuis commented Sep 23, 2016

bnoordhuis commented Sep 23, 2016

src: add /json/protocol endpoint to inspector #7491

src: add /json/protocol endpoint to inspector #7491

Conversation

bnoordhuis commented Jun 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jun 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jun 30, 2016

joshgav commented Jun 30, 2016

bnoordhuis commented Jun 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jun 30, 2016

bnoordhuis commented Jun 30, 2016 • edited Loading

bnoordhuis commented Jul 6, 2016

pavelfeldman commented Jul 6, 2016

joaocgreis commented Jul 7, 2016

bnoordhuis commented Jul 9, 2016

pavelfeldman commented Jul 11, 2016

jasnell commented Aug 9, 2016

bnoordhuis commented Aug 10, 2016

pavelfeldman commented Aug 10, 2016

bnoordhuis commented Aug 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Aug 23, 2016

jasnell commented Aug 23, 2016

cjihrig commented Aug 23, 2016

bnoordhuis commented Sep 11, 2016 • edited Loading

joshgav commented Sep 15, 2016 • edited Loading

jasnell commented Sep 20, 2016

bnoordhuis commented Sep 23, 2016

bnoordhuis commented Sep 23, 2016

bnoordhuis commented Jun 29, 2016 •

edited

Loading

bnoordhuis commented Jun 30, 2016 •

edited

Loading

bnoordhuis commented Sep 11, 2016 •

edited

Loading

joshgav commented Sep 15, 2016 •

edited

Loading