src: fix context inspection for V8 6.8

[mmarchini]

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]

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+.

[joyeecheung]

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

[mmarchini]

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]

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]

@joyeecheung any thoughts on #201 (comment)?

[joyeecheung]

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

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

[mmarchini]

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

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.

[mmarchini]

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]

Ping @joyeecheung

[mmarchini]

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