Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: fix context inspection for V8 6.8 #201

Closed
wants to merge 6 commits into from

Conversation

mmarchini
Copy link
Contributor

@mmarchini mmarchini commented May 22, 2018

ETA: July 24th (after V8 6.8 lands on nodejs/node)

V8 6.8 replaces Closure inside Context with ScopeInfo. Those are minimal
changes to adopt llnode to the new behavior.

Ref: chromium-review.googlesource.com/#/c/785151
Fixes: #193

@mmarchini
Copy link
Contributor Author

There's still one problem though: since closure was removed from Context, there's no (easy) way to get the outer JsFunction from the Context or the ScopeInfo.

We could either:

  • (a) iterate the heap to find a JsFunction with the same scope_info as the Context being inspected.
  • (b) Output only the JsFunction name and script info, omitting its address.
  • (c) omit the closure altogether in Context::Inspect for V8 6.8+.

@mmarchini mmarchini changed the title V8 6.8 fix [WIP] src: fix context inpection for V8 6.8 May 22, 2018
@joyeecheung
Copy link
Member

Maybe we can do (b by default and add an option for (a?

@mmarchini
Copy link
Contributor Author

mmarchini commented Jul 4, 2018

Ok, I've been reading further on why the closure field was removed (v8/v8:7066) and turns out it was a memory leak vector, especially in IIFE. With the changes on V8 the closure might be garbage collected, thus I don't think we should try to guess it or add new post-mortem metadata to V8 to inspect it. IMO we should just replace the closure with scope_info in the inspect for V6.8+. The caller can be easily spotted with v8 bt, and we can use #211 + the (previous) context to inspect the outer scope.

@mmarchini
Copy link
Contributor Author

Before 6.8, inspecting functions look like this:

0x00000f4f74f6e979:<function: method at /Users/mmarchini/workspace/nodejs/llnode/test/fixtures/inspect-scenario.js:27:43
  context=0x00000f4fded9e541{
    (previous)=0x00000f4f85482f11
    (closure)=0x00000f4f85482e59 {<function: closure at /Users/mmarchini/workspace/nodejs/llnode/test/fixtures/inspect-scenario.js:18:17>},
    scopedVar=0x00000f4f74f6dc51:<String: "scoped value">,
    scopedAPI=0x00000f4fdeda0e11:<Object: Zlib>,
    scopedArray=0x00000f4fdeda2011:<Array: length=2>}>

After 6.8 it would look like this:

(llnode) v8 inspect 0x000024951cb43e99
0x000024951cb43e99:<function: method at /Users/mmarchini/workspace/nodejs/llnode/test/fixtures/inspect-scenario.js:27:43
  context=0x00002495f5edca69{
    (previous)=0x00002495f5e9dfa9
    (scope_info)=0x000024951cb43191,
    scopedVar=0x000024951cb43169:<String: "scoped value">,
    scopedAPI=0x00002495f5edf399:<Object: Zlib>,
    scopedArray=0x00002495f5ee0599:<Array: length=2>}>

@mmarchini
Copy link
Contributor Author

@joyeecheung any thoughts on #201 (comment)?

@joyeecheung
Copy link
Member

joyeecheung commented Jul 12, 2018

@mmarchini Displaying scope info is better than nothing, but does that mean now we have no way of knowing where (in the souce code) the context is coming from if we are simply inspecting a context?

@joyeecheung
Copy link
Member

Looks like there are still InferredFunctionName() and FunctionName() in ScopeInfo(), but I couldn't find any thing that leads to the Script

@mmarchini
Copy link
Contributor Author

@joyeecheung yeah, we can get the function name from a ScopeInfo, but not the Script. I'm working on this, #211 and #195, and so far this is the most detailed output I could get with 6.8 when inspecting a function:

(llnode) v8 inspect 0x00002e16142e8f79
0x00002e16142e8f79:<function: method at /Users/mmarchini/workspace/nodejs/llnode/test/fixtures/inspect-scenario.js:27:43
  context=0x00002e16400e9799:<Context: {
    (previous)=0x00002e16400ab199:<Context>,
    (scope_info)=0x00002e16142e8271:<ScopeInfo: for function closure>,
    scopedVar=0x00002e16142e8249:<String: "scoped value">,
    scopedAPI=0x00002e16400ec109:<Object: Zlib>,
    scopedArray=0x00002e16400ed309:<Array: length=2> }>>

I think I'll move forward with this approach and we can try to improve the situation later, maybe using some heuristic to try to find the script for the ScopeInfo.

@mmarchini mmarchini changed the title [WIP] src: fix context inpection for V8 6.8 src: fix context inpection for V8 6.8 Jul 27, 2018
@mmarchini mmarchini changed the title src: fix context inpection for V8 6.8 src: fix context inspection for V8 6.8 Jul 31, 2018
@mmarchini
Copy link
Contributor Author

I created a POC to inspect Context objects, and I think this would be enough to cover the current use case (we wouldn't know the script and line where the closure was defined, but this information can be inferred from the name + script of the current function).

Here's what we would do today to inspect the current function and the closure:

https://asciinema.org/a/BPvhOsMdv3aMRv1E5nYXWOdjp

Here is the proposed solution (instead of inspecting the closure, inspect the previous context to find out which variables can be accessed in the current function):

https://asciinema.org/a/pvS0iL81vtbjhZUijMMUpqW1j

Am I missing any use cases? If this solution is acceptable I'll update our tests to fit it and add some more tests.

Matheus Marchini added 4 commits August 20, 2018 13:43
V8 6.8 replaces Closure inside Context with ScopeInfo. Those are minimal
changes to adopt llnode to the new behavior.

Ref: https://chromium-review.googlesource.com/#/c/785151/
Fixes: nodejs#193
This patch teaches llnode how to inspect Context objects. This is useful
to inspect context variables.

Fix: nodejs#211
@mmarchini
Copy link
Contributor Author

CI is happy with the latest changes 😄
(but shhhh, let's keep it quiet otherwise we might scare it away)

P.S.: The last commit also fixes #211

@mmarchini
Copy link
Contributor Author

Ping @joyeecheung

joyeecheung

This comment was marked as off-topic.

mmarchini pushed a commit that referenced this pull request Aug 31, 2018
V8 6.8 replaces Closure inside Context with ScopeInfo. Those are minimal
changes to adopt llnode to the new behavior.

Ref: https://chromium-review.googlesource.com/#/c/785151/
Fixes: #193

PR-URL: #201
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
mmarchini pushed a commit that referenced this pull request Aug 31, 2018
This patch teaches llnode how to inspect Context objects. This is useful
to inspect context variables.

PR-URL: #201
Fixes: #211
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@mmarchini
Copy link
Contributor Author

Landed in d348eff...d5e0ada. Thanks for the feedback!

@mmarchini mmarchini closed this Aug 31, 2018
hyj1991 pushed a commit to aliyun-node/llnode that referenced this pull request Sep 3, 2018
V8 6.8 replaces Closure inside Context with ScopeInfo. Those are minimal
changes to adopt llnode to the new behavior.

Ref: https://chromium-review.googlesource.com/#/c/785151/
Fixes: nodejs#193

PR-URL: nodejs#201
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
hyj1991 pushed a commit to aliyun-node/llnode that referenced this pull request Sep 3, 2018
This patch teaches llnode how to inspect Context objects. This is useful
to inspect context variables.

PR-URL: nodejs#201
Fixes: nodejs#211
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Drieger pushed a commit to Drieger/llnode that referenced this pull request Sep 4, 2018
V8 6.8 replaces Closure inside Context with ScopeInfo. Those are minimal
changes to adopt llnode to the new behavior.

Ref: https://chromium-review.googlesource.com/#/c/785151/
Fixes: nodejs#193

PR-URL: nodejs#201
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Drieger pushed a commit to Drieger/llnode that referenced this pull request Sep 4, 2018
This patch teaches llnode how to inspect Context objects. This is useful
to inspect context variables.

PR-URL: nodejs#201
Fixes: nodejs#211
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants