Be careful not to leak with event listeners

[Justin Lebar]

Dear all,

We've been working on some B2G memory leaks lately, and so many of the
ones we found have had to do with event listeners, I thought it
warranted an e-mail to the list.

The essential problem is that an event listener registered on DOM node
N lives for as long as N lives. If N is long-lived (e.g. it's the
window of the system app), then the event listener will also live for
a long time.

A closure keeps alive everything it can possibly touch. So if you
touch e.g. a DOM node from within an event listener, that node and all
of its ancestors and descendents will stay alive at least until the
event listener dies.

For example, this would cause |iframe| to stay alive for as long as
|window| lives, even if you remove all other pointers to |iframe|

var iframe = ...;
window.addEventListener('mozbrowserfoo', function(e) {
iframe.setVisible(false);
});

because the mozbrowserfoo event listener lives as long as the window
lives, and keeps the iframe alive. It's particularly bad to leak an
<iframe mozbrowser> because we then leak all the mozbrowser machinery
for that iframe. A solution in this case might be to register the
event listener on |iframe| itself, since then the event listener lives
only as long as the iframe lives, and therefore doesn't extend the
iframe's lifetime. Alternatively, you could pull |iframe| off
e.target.

One of the bugs we fixed did this:

var homescreenIframe = ...;
window.addEventListener('something', function(e) {
document.doSomething();
});

This leaked homescreenIframe, even though the closure did not touch
homescreenIframe. The work-around in this case was to pull |document
off e.target.ownerDocument.

We believe this may be a bug in the JS engine (bug 894149), and we're
working on a fix. For now, please be cautious of touching |document|
or |window| from within an event listener: You may end up keeping many
things alive that you don't mean to.

If you have any questions or doubts, feel free to ask me, khuey, or
mccr8. This stuff very tricky to do right, and unfortunately these
leaks are difficult to notice and fix, so a bit of prevention may go a
long way.

Regards,
-Justin

https://bugzilla.mozilla.org/show_bug.cgi?id=894135
https://bugzilla.mozilla.org/show_bug.cgi?id=894147
https://bugzilla.mozilla.org/show_bug.cgi?id=894081

[Justin Lebar]

> For now, please be cautious of touching |document|
> or |window| from within an event listener: You may end up keeping many
> things alive that you don't mean to.

We may have figured this bit out.

The good news is, we now don't think that you'll cause leaks by
touching |document| or |window| from within a closure.

The bad news is that there's a new way to leak that you should be aware of.

Suppose function F contains two functions, f1, and f2, and suppose the
set of variables touched by f1 is C1, and the set of variables touched
by f2 is C2. Then so long as either f1 or f2 is alive, all variables
in C1 union C2 are kept alive!

That is to say, all closures within a given function share the same
scope. As an example:

function foo() {
var v1, v2, v3;

setTimeout(function f1() {
dump(v1);
}, 0);

addEventListener("bar", function f2() {
dump(v2);
});
}

f1 and f2 each keep alive /both/ v1 and v2. So even after the
setTimeout runs, f2 continues to hold alive v2 /and/ v1.

The scheme under which f1 keeps alive only v1 and f2 keeps alive only
v2 is called a "safe-for-space closure". It's apparently difficult to
do right, so we shouldn't expect JS engines to do this anytime soon.
See http://flint.cs.yale.edu/flint/publications/escc.html.

Happy hacking,
-Justin

[Jared Wein]

This reminds me of this blog post I read a couple weeks ago, http://point.davidglasser.net/2013/06/27/surprising-javascript-memory-leak.html.

Hope that helps,
Jared

[Justin Lebar]

On Tue, Jul 16, 2013 at 4:49 PM, Felipe G <fel...@gmail.com> wrote:
>> The bad news is that there's a new way to leak that you should be aware
>> of.
>

> Why do you say this is a new way to leak? Did this not leak before, and the
> fix for the |document| leak will introduce it?

It's only "new to me" -- there's no fix for the |document| leak (it
probably was a figment of our collective imagination), and afaik the
closure behavior has been in place for a long time.

>> > working on a fix. For now, please be cautious of touching |document|

>> > or |window| from within an event listener: You may end up keeping many
>> > things alive that you don't mean to.
>> >

>> > If you have any questions or doubts, feel free to ask me, khuey, or
>> > mccr8. This stuff very tricky to do right, and unfortunately these
>> > leaks are difficult to notice and fix, so a bit of prevention may go a
>> > long way.
>> >
>> > Regards,
>> > -Justin
>> >
>> > https://bugzilla.mozilla.org/show_bug.cgi?id=894135
>> > https://bugzilla.mozilla.org/show_bug.cgi?id=894147
>> > https://bugzilla.mozilla.org/show_bug.cgi?id=894081

>> _______________________________________________
>> firefox-dev mailing list
>> firef...@mozilla.org
>> https://mail.mozilla.org/listinfo/firefox-dev
>
>

[Felipe G]

> The bad news is that there's a new way to leak that you should be aware
of.

Why do you say this is a new way to leak? Did this not leak before, and the
fix for the |document| leak will introduce it?

[L. David Baron]

On Tuesday 2013-07-16 16:51 -0700, Justin Lebar wrote:
> On Tue, Jul 16, 2013 at 4:49 PM, Felipe G <fel...@gmail.com> wrote:

> >> The bad news is that there's a new way to leak that you should be aware
> >> of.
> >
> > Why do you say this is a new way to leak? Did this not leak before, and the
> > fix for the |document| leak will introduce it?
>

> It's only "new to me" -- there's no fix for the |document| leak (it
> probably was a figment of our collective imagination), and afaik the
> closure behavior has been in place for a long time.

For what it's worth, I think the old behavior was substantially
*leakier*. When I wrote
http://www.mozilla.org/scriptable/avoiding-leaks.html in 2005, which
has now been migrated to
https://developer.mozilla.org/en-US/docs/Using_XPCOM_in_JavaScript_without_leaking
I believe the behavior was that all the variables in the containing
scope were entrained by the closure, whether the closure used them
or not. Limiting this to only variables used by some closure is a
more recent optimization.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla http://www.mozilla.org/ 𝄂

[David Bruant]

2013/7/17 Justin Lebar <justin...@gmail.com>

> Suppose function F contains two functions, f1, and f2, and suppose the
> set of variables touched by f1 is C1, and the set of variables touched
> by f2 is C2. Then so long as either f1 or f2 is alive, all variables
> in C1 union C2 are kept alive!
>

> (...)

>
> The scheme under which f1 keeps alive only v1 and f2 keeps alive only
> v2 is called a "safe-for-space closure". It's apparently difficult to
> do right, so we shouldn't expect JS engines to do this anytime soon.
> See http://flint.cs.yale.edu/flint/publications/escc.html.
>

Fascinating...
You say "JS engines". Have you noticed the same leak in other JS engines?

David

[David Bruant]

2013/7/17 L. David Baron <dba...@dbaron.org>

It seems that most of what you wrote under
Using_XPCOM_in_JavaScript_without_leaking<https://developer.mozilla.org/en-US/docs/Using_XPCOM_in_JavaScript_without_leaking>applies
to general-purpose JS. Do you mind if I (or anyone) move that
content to a more generic MDN page about JS memory management?

Since we're talking about memory management in JS, I encourage all Gaia/web
devs to read:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management(there
is some overlap with the Usign XPCOM in JS without leaking page)
I also recently gave a JS dev-oriented talk on the topic with some pretty
drawing of graphs:
https://www.youtube.com/watch?v=ADiF5UUKDCk

Thanks,

David

[Tim Chien]

Hi Justin!

On Wed, Jul 17, 2013 at 12:48 AM, Justin Lebar <justin...@gmail.com> wrote:
> One of the bugs we fixed did this:
>
> var homescreenIframe = ...;
> window.addEventListener('something', function(e) {
> document.doSomething();
> });
>
> This leaked homescreenIframe, even though the closure did not touch
> homescreenIframe. The work-around in this case was to pull |document
> off e.target.ownerDocument.

I understand the |homescreenIframe| will be kept alive we remove the
event listener of 'something' here,
but I don't understand why the workaround would be to use
|e.target.ownerDocument| instead of |document|.

If 'something' is a one-time thing, shouldn't be the proper fix would
be try to remove the event listener as soon as possible? For example,
the 'load' event for window ...

--
Tim Guan-tin Chien, Engineering Manager and Front-end Lead, Firefox
OS, Mozilla Corp. (Taiwan)

[Kyle Huey]

Hi Tim,

On Wed, Jul 17, 2013 at 1:38 AM, Tim Chien <timd...@mozilla.com> wrote:

> I understand the |homescreenIframe| will be kept alive we remove the
> event listener of 'something' here,
> but I don't understand why the workaround would be to use
> |e.target.ownerDocument| instead of |document|.
>

We were wrong, replacing |document| didn't change anything. Justin's
second email explains what is really going on.

- Kyle

[Florian Bender]

Am 17.07.2013 um 01:51 schrieb Justin Lebar:
> Suppose function F contains two functions, f1, and f2, and suppose the
> set of variables touched by f1 is C1, and the set of variables touched
> by f2 is C2. Then so long as either f1 or f2 is alive, all variables
> in C1 union C2 are kept alive!
>

[snip]

>
> f1 and f2 each keep alive /both/ v1 and v2. So even after the
> setTimeout runs, f2 continues to hold alive v2 /and/ v1.

What does that mean WRT IIFE as a way to encapsulate scripts / libraries?
If you use a library (let's say jQuery because it's huge and most sites only
use a fraction of the functions declared in it), and all functions inside (both
internal and exported functions) are declared by function expressions (so
being tied to a variable), and there's a listener for e. g. onDOMReady, the
whole library leaks?

Can this be mitigated by using function declarations? How does lazy
parsing help here? Or are functions not affected at all (instances are,
obviously, aren't they)?

Best regards,

Florian Bender

quantumedia · Kalchthaler, Müller & Partner Media Services GbR
Münsterplatz 32 · 79098 Freiburg · Tel. 0761 458 99 28-0 · Fax 0761 458 99 28-9
USt-IdNr: DE280390014 · Geschäftsführer: Gregor Kalchthaler, Philipp Müller

[Kyle Huey]

On Tue, Jul 16, 2013 at 3:05 PM, Justin Lebar <justin...@gmail.com>wrote:

> The scheme under which f1 keeps alive only v1 and f2 keeps alive only
> v2 is called a "safe-for-space closure". It's apparently difficult to
> do right, so we shouldn't expect JS engines to do this anytime soon.
> See http://flint.cs.yale.edu/flint/publications/escc.html.
>

I filed bug 894971 on optimizing this further with some ideas of things we
could short of true "safe-for-space" closures. Brian Hackett also wrote an
analysis that points out these sort of issues in bug 894669. We're going
to look at the output that gives on Gaia and see if it's manageable.

- Kyle