inspector: support extra contexts when connected

[jgoz]

Checklist

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

Affected core subsystem(s)

inspector

This makes the inspector aware of extra contexts, enabling debugging inside vm.Script code.

It is based on @eugeneo's branch, which used to have an issue with GC crashes due to InspectedContexts not being destroyed in the right order, but this is no longer an issue as of V8 5.9.

The original approach sent every context to the inspector whether or not it was connected, but wrapping a context in InspectedContext has a side-effect of attaching global and console instances to the context, which caused test failures. The approach I took was to only call contextCreated if the inspector is actually connected to limit the impact of these side effects.

Fixes: #7593
Fixes: #12096
Closes: #9272

[jgoz]

Also note that due to the aforementioned GC issue in <5.9, this can't be backported prior releases without significant work (I think).

[eugeneo]

Checked with V8 developers, this should not work. Let me test this.

[eugeneo]

I am still debugging - looks like the contexts are never released (e.g. have you seen the contextDestroyed protocol notification?). I added print statements all over the place and they are only called for the main context when the application runs to a completion. I tried forcing GC, creating large arrays, etc.

Also, this code does not seem to work with sessions created through JavaScript because the IO is never started. I don't think there's a need to maintain a separate vector of contexts - inspector already does that if you call context Created/Destroyed.

[jgoz]

I am still debugging - looks like the contexts are never released (e.g. have you seen the contextDestroyed protocol notification?).

Hmm, that's a problem. So the WeakCallback check that used to be in 5.8 but is no longer in 5.9 would have caught these. Any ideas on how to destroy these properly?

Also, this code does not seem to work with sessions created through JavaScript because the IO is never started. I don't think there's a need to maintain a separate vector of contexts - inspector already does that if you call context Created/Destroyed.

I see. I added that to avoid polluting all vm contexts with global and console, which gets added by InspectedContext. It was causing vm-test-basic to fail because the contexts had these extra properties attached to them. I'm not really sure how else to deal with that.

[eugeneo]

Ok, looks like it is actually Inspector who retains the reference to the Context. So always reporting the context actually prevents it from being garbage collected.

Background info: browser has full information when the context will be destroyed (page navigation, etc). Hence it can warn the inspector to stop tracking the context which frees the reference. Unfortunately, this approach does not work with Node as it relies on contexts being GCed.

[eugeneo]

Hmm, that's a problem. So the WeakCallback check that used to be in 5.8 but is no longer in 5.9 would have caught these. Any ideas on how to destroy these properly?

That's the problem - to release those you need to call contextDestroyed. But Node is calling contextDestroyed from the GC callback...

I see. I added that to avoid polluting all vm contexts with global and console, which gets added by InspectedContext. It was causing vm-test-basic to fail because the contexts had these extra properties attached to them. I'm not really sure how else to deal with that.

I will raise this issue with the V8 developers.

[jgoz]

That's the problem - to release those you need to call contextDestroyed. But Node is calling contextDestroyed from the GC callback...

Makes sense.

Ok, looks like it is actually Inspector who retains the reference to the Context. So always reporting the context actually prevents it from being garbage collected.

Background info: browser has full information when the context will be destroyed (page navigation, etc). Hence it can warn the inspector to stop tracking the context which frees the reference. Unfortunately, this approach does not work with Node as it relies on contexts being GCed.

OK, and as you said, for this to work we have to always report contexts (rather than only reporting when connected) to ensure sessions created from JS still work.

It sounds to me like this feature depends on either upstream changes in V8 or bigger changes around Node's expectation that contexts will be GCed. Is there anything salvageable from this PR, or should we just close it?

[eugeneo]

For your ref this is the change that made inspector retain the context - v8/v8@908cd38

Re: this PR - I would suggest you to close it but keep your branch.

Thanks!

[jgoz]

OK, thanks for the review & background info!