Bug 106813 – [V8] V8PerContextData objects' lifetimes should match their V8Context objects

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Matthew Dempsky]

V8DOMWindowShell creates V8Context and V8PerContextData objects, but while V8Context objects may outlive the window (e.g., by being referenced by an object passed to another window), V8PerContextData objects are destroyed as soon as the V8DOMWindowShell is destroyed.  This causes V8PerContextData::from() to return nullptr, and in turn causes code npCreateV8ScriptObject() to create multiple NPObject* values for the same V8 object.

[Matthew Dempsky]

Created attachment 182608 [details]
Patch

[Adam Barth]

Comment on attachment 182608 [details]
Patch

Looks great.

[Matthew Dempsky]

Hm, perhaps it's not this simple.  E.g., fast/workers/worker-document-leak.html seems to fail consistently with my patch applied, saying that all 6 of the iframe documents it creates are still alive.

I suspect the other fields in V8PerContextData are transitively keeping the v8::Context alive.

They could perhaps all be made weak, but that seems ugly and fragile.  I don't really grok V8's GC scheme well enough to suggest better solutions though.

[Adam Barth]

Comment on attachment 182608 [details]
Patch

ok

[Matthew Dempsky]

I think the cycles can be eliminated by removing the v8::Persistent handles from V8PerContextData.  Instead, it will need to store its handles inside a v8::Object that in turn is stored in the v8::Context with SetEmbedderData.

A single HashMap<WrapperTypeInfo*,unsigned> can be used for converting WrapperTypeInfo* values into ordinal values, and then used to index into the v8::Object backing store.  m_errorPrototype is at index 0, m_objectPrototype is at index 1, and wrapper objects and constructors are indexes i*2 and i*2+1 respectively.

The V8NPObjectMap can be left as is because although it contains v8::Persistent handles too (in the V8NPObjects), those values already have their lifetimes bound to their NPObject wrappers instead.  Once the NPObjects are released, the V8NPObject will be removed automatically.

Then I think the weak v8::Context handle will actually work as intended.

I'm going to work on implementing this to see if it works.  If anyone objects to the idea or can suggest something better, I'm interested.

[Adam Barth]

Another thing we need to be careful about is that if the lifetime of V8PerContextData is controlled by the garbage collector, then we need to be prepared for the V8PerContextData object to be destroyed any time garbage collection can run.  Perhaps we need a way for folks who use V8PerContextData keep it alive?  Perhaps if they always have a local handle to the v8::Context that will be enough...

[Matthew Dempsky]

Good point.  Since the only way you can get a V8PerContextData is through V8PerContextData::from(v8::Handle<v8::Context>), perhaps we can just document that the caller is responsible for keeping the context alive as long as they're using the V8PerContextData?  I think that's already true for all existing users of V8PerContextData.

[Adam Barth]

That seems reasonable.

[Anders Carlsson]

V8 is gone from WebKit.