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
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P1 Normal
Assigned To:
:
: HasReduction, InRadar, ReviewedForRadar
:
: 25370
  Show dependency treegraph
 
Reported: 2008-09-30 17:03 PST by
Modified: 2009-04-20 20:57 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-09-30 17:03:07 PST
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 From 2008-09-30 17:03:37 PST -------
Created an attachment (id=23964) [details]
Testcase
------- Comment #2 From 2008-09-30 17:37:09 PST -------
<rdar://problem/6259524>
------- Comment #3 From 2008-10-01 23:28:41 PST -------
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 From 2008-10-01 23:31:58 PST -------
Another similar case:

someElement.onfoo = someFunction;
someFunction.bar = someElement;
------- Comment #5 From 2008-10-01 23:53:43 PST -------
I think we could make each DOM object responsible for marking its event listeners.
------- Comment #6 From 2008-10-01 23:55:23 PST -------
I believe I know how to fix this bug.  We should just eliminate the protected listener concept and use custom marking.
------- Comment #7 From 2009-03-25 13:38:16 PST -------
Maybe related: bug 24806.
------- Comment #8 From 2009-03-26 14:02:45 PST -------
Created an attachment (id=28984) [details]
reduction
------- Comment #9 From 2009-04-06 13:10:46 PST -------
Created an attachment (id=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 From 2009-04-06 13:16:01 PST -------
(From update of attachment 29286 [details])
> -    // 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 From 2009-04-06 13:22:04 PST -------
> 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 From 2009-04-06 14:57:18 PST -------
Committed revision 42256.
------- Comment #13 From 2009-04-13 11:54:03 PST -------
Fixed, right?
------- Comment #14 From 2009-04-17 16:17:33 PST -------
Created an attachment (id=29592) [details]
Next patch

Another in the long stream of patches for this bug.
------- Comment #15 From 2009-04-17 16:17:50 PST -------
Still some work to be done on this bug - reopening for patch review.
------- Comment #16 From 2009-04-17 18:32:20 PST -------
Committed revision r42633. Still a little more to go, though.
------- Comment #17 From 2009-04-19 00:15:54 PST -------
Created an attachment (id=29603) [details]
Next patch
------- Comment #18 From 2009-04-19 00:30:33 PST -------
(From update of attachment 29603 [details])
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 From 2009-04-19 00:39:34 PST -------
Committed revision r42657. Still a little more to go, though.
------- Comment #20 From 2009-04-19 23:46:57 PST -------
Created an attachment (id=29610) [details]
last patch, i promise
------- Comment #21 From 2009-04-20 00:15:05 PST -------
(From update of attachment 29610 [details])
> +    // 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 From 2009-04-20 09:06:15 PST -------
(From update of attachment 29610 [details])
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 From 2009-04-20 09:10:03 PST -------
> > +    // 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 From 2009-04-20 09:16:39 PST -------
> 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 From 2009-04-20 20:56:47 PST -------
We settled on "AttributeEventListener."