Bug 106813

Created attachment 182608 [details] Patch

Comment on attachment 182608 [details] Patch Looks great.

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.

Comment on attachment 182608 [details] Patch ok

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.

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

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.

That seems reasonable.

V8 is gone from WebKit.