Bug 21260

Created attachment 23964 [details] Testcase

<rdar://problem/6259524>

Okay, so this isn't a permanent leak -- the functions get collected when the Page goes away. The underlying problem appears to be that the JSEventListener uses a ProtectedPtr to hold the event handler. However because the event handler is defined along the lines of: var a = someElement; a.addEventListener("..", function(){}) The function's Activation picks up 'a'. Then when ever we do a gc sweep that will mark the protected event handler (the function we defined) which will mark its activation, which in turn marks the element. This is a ref cycle which means we should just leak forever, however the Page somehow tracks this and manually breaks the cycle when it gets torn down. To fix this we probably just need to ensure we do correct gc rather than relying on protectedptr on JSEventListener, although i'm honestly not sure how we should do this :-(

Another similar case: someElement.onfoo = someFunction; someFunction.bar = someElement;

I think we could make each DOM object responsible for marking its event listeners.

I believe I know how to fix this bug. We should just eliminate the protected listener concept and use custom marking.

Maybe related: bug 24806.

Created attachment 28984 [details] reduction

Created attachment 29286 [details] patch Fixes the reduction, and most node-based event handler leaks. More work remains to be done, but this patch is a good improvement.

Comment on attachment 29286 [details] patch > - // If we're already marking this tree, then we can simply mark this wrapper > - // by calling the base class; our caller is iterating the tree. > + // Nodes in a subtree are kept alive by the tree's root, so, if the root > + // is already marking the tree, we only need to mark ourselves. > if (root->inSubtreeMark()) { > DOMObject::mark(); > + markEventListeners(node->eventListeners()); > return; > } I find it a little strange that the comment says "only need to mark ourselves" and the code says "call the base class mark and also call markEventListeners". Is there a simple way to make the comment and code consistent? Same issue earlier in the file, in the more complicated version that also calls mark on the document wrapper. r=me

> Is there a simple way to make the comment and code consistent? Changed to "don't need to explicitly mark any other nodes."

Committed revision 42256.

Fixed, right?

Created attachment 29592 [details] Next patch Another in the long stream of patches for this bug.

Still some work to be done on this bug - reopening for patch review.

Committed revision r42633. Still a little more to go, though.

Created attachment 29603 [details] Next patch

Comment on attachment 29603 [details] Next patch r=me > - return JSEventListener::create(asObject(val), this, isInline).get(); > + return JSEventListener::create(asObject(val), this, false).get(); Hopefully, the confusing boolean arguments won't remain here forever.

Committed revision r42657. Still a little more to go, though.

Created attachment 29610 [details] last patch, i promise

Comment on attachment 29610 [details] last patch, i promise > + // Ensure that 'node' has a JavaScript wrapper to mark the event listener we're creating. What keeps it from being collected in the future?

Comment on attachment 29610 [details] last patch, i promise r=me The term "inline" here drives me crazy now. Is there any precedent outside WebKit for calling handlers created with HTML attributes "inline event listeners"?

> > + // Ensure that 'node' has a JavaScript wrapper to mark the event listener we're creating. > > What keeps it from being collected in the future? At GC time, the document marks all node wrappers that are observable through the DOM. Observable qualities include custom properties set on a JS wrapper, and JS event handlers set on a node.

> The term "inline" here drives me crazy now. Is there any precedent outside > WebKit for calling handlers created with HTML attributes "inline event > listeners"? Me too! :) No, I don't think there's a precedent for using the word "inline." But I haven't been able to come up with a replacement word that satisfies everybody. Some options: onEventListener (because the relevant properties all start with "on"); soloEventListener (because you can only have one at a time, unlike the addEventListener API); legacyEventListener (because this is a pseudo-deprecated way to add event listeners). What do you think?

We settled on "AttributeEventListener."