Bug 21260

Summary: Unbounded memory growth when churning elements with anonymous event handler functions
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: WebCore JavaScriptAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: alice.barraclough, ap, ddkilzer, mrowe, oliver, pam, sam
Priority: P1 Keywords: HasReduction, InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 25370    
Attachments:
Description Flags
Testcase
none
reduction
none
patch
darin: review+
Next patch
oliver: review+
Next patch
ap: review+
last patch, i promise darin: review+

Simon Fraser (smfr)
Reported 2008-09-30 17:03:07 PDT
The to-be-attached testcase creates, attaches, then removes hundreds of divs, each of which has an event handler registered on it. If the event handler uses an anonymous function, like: a.addEventListener("click", function () {}, true); then memory grows over time, and is not fully reclaimed by GC. Debugging shows that none of the HTMLElements are destroyed, even after they have been removed from the DOM. 'leaks' does not report any leaks, but the Caches window shows an increasing number of Function Protected Objects. If the event handler uses a named function: function clicked(e) {} a.addEventListener("click", clicked, true); then the problem does not occur. Seen on WebKit TOT r37078. Note that if the elements that are entrained are costly (like <canvas>) then this rapidly becomes a problem.
Attachments
Testcase (2.61 KB, text/html)
2008-09-30 17:03 PDT, Simon Fraser (smfr)
no flags
reduction (284 bytes, text/html)
2009-03-26 14:02 PDT, Geoffrey Garen
no flags
patch (13.63 KB, patch)
2009-04-06 13:10 PDT, Geoffrey Garen
darin: review+
Next patch (108.81 KB, patch)
2009-04-17 16:17 PDT, Geoffrey Garen
oliver: review+
Next patch (10.37 KB, patch)
2009-04-19 00:15 PDT, Geoffrey Garen
ap: review+
last patch, i promise (11.63 KB, patch)
2009-04-19 23:46 PDT, Geoffrey Garen
darin: review+
Simon Fraser (smfr)
Comment 1 2008-09-30 17:03:37 PDT
Created attachment 23964 [details] Testcase
Geoffrey Garen
Comment 2 2008-09-30 17:37:09 PDT
Oliver Hunt
Comment 3 2008-10-01 23:28:41 PDT
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 :-(
Alexey Proskuryakov
Comment 4 2008-10-01 23:31:58 PDT
Another similar case: someElement.onfoo = someFunction; someFunction.bar = someElement;
Geoffrey Garen
Comment 5 2008-10-01 23:53:43 PDT
I think we could make each DOM object responsible for marking its event listeners.
Sam Weinig
Comment 6 2008-10-01 23:55:23 PDT
I believe I know how to fix this bug. We should just eliminate the protected listener concept and use custom marking.
Geoffrey Garen
Comment 7 2009-03-25 13:38:16 PDT
Maybe related: bug 24806.
Geoffrey Garen
Comment 8 2009-03-26 14:02:45 PDT
Created attachment 28984 [details] reduction
Geoffrey Garen
Comment 9 2009-04-06 13:10:46 PDT
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.
Darin Adler
Comment 10 2009-04-06 13:16:01 PDT
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
Geoffrey Garen
Comment 11 2009-04-06 13:22:04 PDT
> Is there a simple way to make the comment and code consistent? Changed to "don't need to explicitly mark any other nodes."
Geoffrey Garen
Comment 12 2009-04-06 14:57:18 PDT
Committed revision 42256.
Simon Fraser (smfr)
Comment 13 2009-04-13 11:54:03 PDT
Fixed, right?
Geoffrey Garen
Comment 14 2009-04-17 16:17:33 PDT
Created attachment 29592 [details] Next patch Another in the long stream of patches for this bug.
Geoffrey Garen
Comment 15 2009-04-17 16:17:50 PDT
Still some work to be done on this bug - reopening for patch review.
Geoffrey Garen
Comment 16 2009-04-17 18:32:20 PDT
Committed revision r42633. Still a little more to go, though.
Geoffrey Garen
Comment 17 2009-04-19 00:15:54 PDT
Created attachment 29603 [details] Next patch
Alexey Proskuryakov
Comment 18 2009-04-19 00:30:33 PDT
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.
Geoffrey Garen
Comment 19 2009-04-19 00:39:34 PDT
Committed revision r42657. Still a little more to go, though.
Geoffrey Garen
Comment 20 2009-04-19 23:46:57 PDT
Created attachment 29610 [details] last patch, i promise
Alexey Proskuryakov
Comment 21 2009-04-20 00:15:05 PDT
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?
Darin Adler
Comment 22 2009-04-20 09:06:15 PDT
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"?
Geoffrey Garen
Comment 23 2009-04-20 09:10:03 PDT
> > + // 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.
Geoffrey Garen
Comment 24 2009-04-20 09:16:39 PDT
> 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?
Geoffrey Garen
Comment 25 2009-04-20 20:56:47 PDT
We settled on "AttributeEventListener."
Note You need to log in before you can comment on or make changes to this bug.