JS DOM wrappers shouldn't need destructors, they all get finalize()'d.
Created attachment 213654 [details] Patch
Comment on attachment 213654 [details] Patch Actually this won't work just yet. :( Problems: * It leaks! Phil mentioned that there's some weird case where we don't run finalizers on a weak block before killing it. * The finalizer shouldn't use jsCast since the object's structure may have been GC'd before the finalizer runs. * JSDOMWrapper should inherit directly from JSNonFinalObject, this will save us 8 bytes per wrapper. * We should emit static assertions that DOM wrappers are trivially destructible.
> * It leaks! Phil mentioned that there's some weird case where we don't run finalizers on a weak block before killing it. O'rly? That sounds like a bug.
(In reply to comment #3) > > * It leaks! Phil mentioned that there's some weird case where we don't run finalizers on a weak block before killing it. > > O'rly? That sounds like a bug. I was hoping you could tell me more about this: <pizlo> yes, it's possible for weak blocks to get thrown out without running finalizers. I think that if you kill the DOMWindow then something like this happens, but I doin't remember the details <pizlo> I remember ggaren and I having a fun fire drill with this about a year ago, there is some stupid case where we totally just say "ok don't need those weak references anymore"
> I was hoping you could tell me more about this: > > <pizlo> yes, it's possible for weak blocks to get thrown out without running finalizers. I think that if you kill the DOMWindow then something like this happens, but I doin't remember the details > <pizlo> I remember ggaren and I having a fun fire drill with this about a year ago, there is some stupid case where we totally just say "ok don't need those weak references anymore" Perhaps Phil was referring to DOMWrapperWorld::clearWrappers()? If you throw away a Weak<T>, it does not run its destructor. So, relying on a Weak<T> to run a destructor is not valid, unless: (a) We change WeakSet::deallocate to run the destructor at deallocation time. (b) We change DOMWrapperWorld and other DOM-y stuff to run the destructor manually before clearing a Weak<T>.
(In reply to comment #5) > Perhaps Phil was referring to DOMWrapperWorld::clearWrappers()? If you throw away a Weak<T>, it does not run its destructor. So, relying on a Weak<T> to run a destructor is not valid, unless: > > (a) We change WeakSet::deallocate to run the destructor at deallocation time. This (intuitively) sounds to me like the way things should work. Why don't they? Because we have to make another sweep over all the weak blocks? > (b) We change DOMWrapperWorld and other DOM-y stuff to run the destructor manually before clearing a Weak<T>. This feels error-prone and hard-to-get-right.
> > (a) We change WeakSet::deallocate to run the destructor at deallocation time. > > This (intuitively) sounds to me like the way things should work. Why don't they? Because we have to make another sweep over all the weak blocks? A destructor can -- and usually does -- invoke WeakSet::deallocate (through ~Weak<T>()). So, it's a little mind-bending to reverse the relationship and say that WeakSet::deallocate also sometimes invokes a destructor. But perhaps you can make this work. Just have WeakSet::deallocate do the equivalent of WeakBlock::finalize(), if and only if the current WeakImpl state is not Finalized. (You need the check to avoid running the destructor twice.) Let me know if the internet still works afterward. > > (b) We change DOMWrapperWorld and other DOM-y stuff to run the destructor manually before clearing a Weak<T>. > > This feels error-prone and hard-to-get-right. I agree.
Comment on attachment 213654 [details] Patch Cleared Sam Weinig's review+ from obsolete attachment 213654 [details] so that this bug does not appear in http://webkit.org/pending-commit.