inspector: overhaul JS binding implementation #15643

TimothyGu · 2017-09-27T20:25:27Z

The first two commits of this PR are identical to #14710. However, the last two commits fixed the GC problem pointed out by @bnoordhuis in #14710 (review), and makes Connection an AsyncWrap for async hooks support.

With

const sess = new inspector.Session();
sess.connect();
sess.post('Runtime.evaluate', {
  expression: 'Promise.resolve(3)',
  awaitPromise: true
}, (err, msg) => {
  console.log(msg);
});

and appropriate async hooks, whereas before this PR it looked like

PROMISE(4): trigger: 1 execution: 1
PROMISE(5): trigger: 4 execution: 1
PROMISE(6): trigger: 4 execution: 1
before:  5
{ result: { type: 'number', value: 3, description: '3' } }
  TickObject(7): trigger: 5 execution: 5
after:   5
before:  6
after:   6
before:  7
after:   7
destroy: 7

after it

INSPECTORJSBINDING(5): trigger: 1 execution: 1
PROMISE(6): trigger: 1 execution: 1
PROMISE(7): trigger: 6 execution: 1
PROMISE(8): trigger: 6 execution: 1
before:  7
  before:  5
{ result: { type: 'number', value: 3, description: '3' } }
    TickObject(9): trigger: 5 execution: 5
  after:   5
after:   7
before:  8
after:   8
before:  9
after:   9
destroy: 9

with proper instrumentation.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

inspector

eugeneo · 2017-09-27T20:30:29Z

Sorry, I am not familiar with the AsyncWrap.

One of the essential requirements for the inspector API is to be able to send messages (and receive responses) while V8 is suspended. Will AsyncWrap work in this scenario?

addaleax · 2017-09-27T20:35:41Z

src/inspector_agent.cc

I think this still needs to be reset in the destructor? It doesn’t happen automatically when ~Persistent() is called, unfortunately…

Very nice spot! It does indeed lead to a memory leak.

addaleax · 2017-09-27T20:37:49Z

One of the essential requirements for the inspector API is to be able to send messages (and receive responses) while V8 is suspended. Will AsyncWrap work in this scenario?

This only changes the code for the inspector JS bindings, so I don’t think that is an issue? The code in this PR calls into JS at the same times when there were calls into JS before.

addaleax · 2017-09-27T20:50:15Z

src/inspector_agent.cc

I think you can leave these out, they are indirectly included by node_internals anyway :)

Doesn't look like that is the case, unfortunately.

- Group all relevant methods/states into a C++ class. - Uses internal fields instead of the less efficient v8::External for storing the pointer to the C++ object. - Use AsyncWrap to allow instrumenting callback states. Refs: nodejs#13503

TimothyGu · 2017-09-28T06:57:48Z

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

Failures unrelated.

trevnorris

Two style nits and one question about a #define. Nothing critical.

trevnorris · 2017-10-02T22:41:09Z

src/inspector_agent.cc

+                       Local<Function> callback)
+                       : AsyncWrap(env, wrap, PROVIDER_INSPECTORJSBINDING),
+                         delegate_(env, this),
+                         callback_(env->isolate(), callback) {


style nit: mind changing the spacing of the initialization list to match that of NodeInspectorClient()?

I thought this was the predominant style for initializers, and I also prefer this style a bit more.

There are other places in the file where the initializer list is in this style. I'd be happy to make this consistent in a separate PR if you insist, but I think this change should get in now separated from stylistic changes..

trevnorris · 2017-10-02T22:47:32Z

src/inspector_agent.cc

+    void SendMessageToFrontend(const v8_inspector::StringView& message)
+        override {
+      Isolate* isolate = env_->isolate();
+      v8::HandleScope handle_scope(isolate);


nit: no need for the v8::; already using v8::HandleScope.

Right. This part of the code was just being moved around, but I'll fix it before landing.

trevnorris · 2017-10-02T22:52:53Z

src/async-wrap.h

  NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V)                                     \
-  NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)
+  NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)                                         \
+  NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V)                                      \


should this end with a \?

Yes, yes it should. Thanks for noticing.

Oh oops, didn't realize it already had a \. I'd say it's good practice to always have a trailing backslash to reduce future diffs, but will remove.

PR-URL: nodejs#15643 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

- Group all relevant methods/states into a C++ class. - Uses internal fields instead of the less efficient v8::External for storing the pointer to the C++ object. - Use AsyncWrap to allow instrumenting callback states. PR-URL: nodejs#15643 Refs: nodejs#13503 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

TimothyGu · 2017-10-03T04:40:17Z

Landed in 7157819 and 2f8ddb2.

PR-URL: nodejs/node#15643 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

- Group all relevant methods/states into a C++ class. - Uses internal fields instead of the less efficient v8::External for storing the pointer to the C++ object. - Use AsyncWrap to allow instrumenting callback states. PR-URL: nodejs/node#15643 Refs: nodejs/node#13503 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-10-07T21:21:04Z

ping re: backport

Backport-PR-URL: nodejs#16071 PR-URL: nodejs#15643 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

- Group all relevant methods/states into a C++ class. - Uses internal fields instead of the less efficient v8::External for storing the pointer to the C++ object. - Use AsyncWrap to allow instrumenting callback states. Backport-PR-URL: nodejs#16071 PR-URL: nodejs#15643 Refs: nodejs#13503 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #16071 PR-URL: #15643 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

- Group all relevant methods/states into a C++ class. - Uses internal fields instead of the less efficient v8::External for storing the pointer to the C++ object. - Use AsyncWrap to allow instrumenting callback states. Backport-PR-URL: #16071 PR-URL: #15643 Refs: #13503 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-11-16T20:24:21Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

TimothyGu requested review from addaleax, bnoordhuis and eugeneo September 27, 2017 20:25

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 27, 2017

addaleax approved these changes Sep 27, 2017

View reviewed changes

eugeneo approved these changes Sep 27, 2017

View reviewed changes

addaleax reviewed Sep 27, 2017

View reviewed changes

mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Sep 27, 2017

TimothyGu removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 27, 2017

TimothyGu added 2 commits September 27, 2017 23:50

module: add inspector to builtinLibs

9eff6d6

inspector: reimplement JS binding

6030094

- Group all relevant methods/states into a C++ class. - Uses internal fields instead of the less efficient v8::External for storing the pointer to the C++ object. - Use AsyncWrap to allow instrumenting callback states. Refs: nodejs#13503

TimothyGu force-pushed the inspector branch from a6c4f41 to 6030094 Compare September 28, 2017 06:56

trevnorris approved these changes Oct 2, 2017

View reviewed changes

jasnell approved these changes Oct 3, 2017

View reviewed changes

TimothyGu closed this Oct 3, 2017

TimothyGu deleted the inspector branch October 3, 2017 04:40

MylesBorins added the backport-requested-v8.x label Oct 3, 2017

TimothyGu mentioned this pull request Oct 7, 2017

[v8.x backport] inspector: overhaul JS binding implementation #16071

Closed

2 tasks

lance added backported-to-v8.x and removed backport-requested-v8.x labels Oct 28, 2017

TimothyGu mentioned this pull request Nov 7, 2017

builtinLibs has not inspector module #14733

Closed

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

TimothyGu added dont-land-on-v6.x and removed backport-requested-v6.x labels Nov 16, 2017

Uh oh!

inspector: overhaul JS binding implementation #15643

inspector: overhaul JS binding implementation #15643

Uh oh!

Conversation

TimothyGu commented Sep 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

eugeneo commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Oct 3, 2017

Uh oh!

MylesBorins commented Oct 7, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

TimothyGu commented Sep 28, 2017 •

edited

Loading