Bug 21260 – Unbounded memory growth when churning elements with anonymous event handler functions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 23964 [details]
Testcase

[Geoffrey Garen]

<rdar://problem/6259524>

[Oliver Hunt]

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]

Another similar case:

someElement.onfoo = someFunction;
someFunction.bar = someElement;

[Geoffrey Garen]

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

[Sam Weinig]

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

[Geoffrey Garen]

Maybe related: bug 24806.

[Geoffrey Garen]

Created attachment 28984 [details]
reduction

[Geoffrey Garen]

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 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]

> 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]

Committed revision 42256.

[Simon Fraser (smfr)]

Fixed, right?

[Geoffrey Garen]

Created attachment 29592 [details]
Next patch

Another in the long stream of patches for this bug.

[Geoffrey Garen]

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

[Geoffrey Garen]

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

[Geoffrey Garen]

Created attachment 29603 [details]
Next patch

[Alexey Proskuryakov]

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]

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

[Geoffrey Garen]

Created attachment 29610 [details]
last patch, i promise

[Alexey Proskuryakov]

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 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]

> > +    // 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]

> 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]

We settled on "AttributeEventListener."