Bug 21260 - Unbounded memory growth when churning elements with anonymous event handler functions
: Unbounded memory growth when churning elements with anonymous event handler f...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P1 Normal
Assigned To: Geoffrey Garen
: HasReduction, InRadar, ReviewedForRadar
Depends on:
Blocks: 25370
  Show dependency treegraph
 
Reported: 2008-09-30 17:03 PDT by Simon Fraser (smfr)
Modified: 2009-04-20 20:57 PDT (History)
7 users (show)

See Also:


Attachments
Testcase (2.61 KB, text/html)
2008-09-30 17:03 PDT, Simon Fraser (smfr)
no flags Details
reduction (284 bytes, text/html)
2009-03-26 14:02 PDT, Geoffrey Garen
no flags Details
patch (13.63 KB, patch)
2009-04-06 13:10 PDT, Geoffrey Garen
darin: review+
Details | Formatted Diff | Diff
Next patch (108.81 KB, patch)
2009-04-17 16:17 PDT, Geoffrey Garen
oliver: review+
Details | Formatted Diff | Diff
Next patch (10.37 KB, patch)
2009-04-19 00:15 PDT, Geoffrey Garen
ap: review+
Details | Formatted Diff | Diff
last patch, i promise (11.63 KB, patch)
2009-04-19 23:46 PDT, Geoffrey Garen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 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.
Comment 1 Simon Fraser (smfr) 2008-09-30 17:03:37 PDT
Created attachment 23964 [details]
Testcase
Comment 2 Geoffrey Garen 2008-09-30 17:37:09 PDT
<rdar://problem/6259524>
Comment 3 Oliver Hunt 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 :-(
Comment 4 Alexey Proskuryakov 2008-10-01 23:31:58 PDT
Another similar case:

someElement.onfoo = someFunction;
someFunction.bar = someElement;
Comment 5 Geoffrey Garen 2008-10-01 23:53:43 PDT
I think we could make each DOM object responsible for marking its event listeners.
Comment 6 Sam Weinig 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.
Comment 7 Geoffrey Garen 2009-03-25 13:38:16 PDT
Maybe related: bug 24806.
Comment 8 Geoffrey Garen 2009-03-26 14:02:45 PDT
Created attachment 28984 [details]
reduction
Comment 9 Geoffrey Garen 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.
Comment 10 Darin Adler 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
Comment 11 Geoffrey Garen 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."
Comment 12 Geoffrey Garen 2009-04-06 14:57:18 PDT
Committed revision 42256.
Comment 13 Simon Fraser (smfr) 2009-04-13 11:54:03 PDT
Fixed, right?
Comment 14 Geoffrey Garen 2009-04-17 16:17:33 PDT
Created attachment 29592 [details]
Next patch

Another in the long stream of patches for this bug.
Comment 15 Geoffrey Garen 2009-04-17 16:17:50 PDT
Still some work to be done on this bug - reopening for patch review.
Comment 16 Geoffrey Garen 2009-04-17 18:32:20 PDT
Committed revision r42633. Still a little more to go, though.
Comment 17 Geoffrey Garen 2009-04-19 00:15:54 PDT
Created attachment 29603 [details]
Next patch
Comment 18 Alexey Proskuryakov 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.
Comment 19 Geoffrey Garen 2009-04-19 00:39:34 PDT
Committed revision r42657. Still a little more to go, though.
Comment 20 Geoffrey Garen 2009-04-19 23:46:57 PDT
Created attachment 29610 [details]
last patch, i promise
Comment 21 Alexey Proskuryakov 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?
Comment 22 Darin Adler 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"?
Comment 23 Geoffrey Garen 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.
Comment 24 Geoffrey Garen 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?
Comment 25 Geoffrey Garen 2009-04-20 20:56:47 PDT
We settled on "AttributeEventListener."