Be careful not to leak with event listeners

153 views
Skip to first unread message

Justin Lebar

unread,
Jul 16, 2013, 12:48:43 PM7/16/13
to <dev-gaia@lists.mozilla.org>, Kyle Huey, Andrew McCreight
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

unread,
Jul 16, 2013, 6:05:08 PM7/16/13
to <dev-gaia@lists.mozilla.org>, Firefox Dev, Kyle Huey, Andrew McCreight
> 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

unread,
Jul 16, 2013, 7:25:14 PM7/16/13
to Justin Lebar, Kyle Huey, dev-gaia, Andrew McCreight, Firefox Dev
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

unread,
Jul 16, 2013, 7:51:11 PM7/16/13
to Felipe G, Kyle Huey, <dev-gaia@lists.mozilla.org>, Andrew McCreight, Firefox Dev
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

unread,
Jul 16, 2013, 7:49:40 PM7/16/13
to Justin Lebar, Kyle Huey, <dev-gaia@lists.mozilla.org>, Andrew McCreight, Firefox Dev
> 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

unread,
Jul 16, 2013, 8:17:29 PM7/16/13
to Justin Lebar, Kyle Huey, Felipe G, <dev-gaia@lists.mozilla.org>, Andrew McCreight, Firefox Dev
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

unread,
Jul 17, 2013, 3:44:13 AM7/17/13
to Justin Lebar, Kyle Huey, <dev-gaia@lists.mozilla.org>, Andrew McCreight, Firefox Dev
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

unread,
Jul 17, 2013, 3:50:54 AM7/17/13
to L. David Baron, Kyle Huey, Justin Lebar, Andrew McCreight, Firefox Dev, Felipe G, <dev-gaia@lists.mozilla.org>
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

unread,
Jul 17, 2013, 4:38:56 AM7/17/13
to Justin Lebar, Kyle Huey, <dev-gaia@lists.mozilla.org>, Andrew McCreight
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

unread,
Jul 17, 2013, 9:45:48 AM7/17/13
to Tim Chien, Andrew McCreight, <dev-gaia@lists.mozilla.org>, Justin Lebar
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

unread,
Jul 17, 2013, 4:53:12 AM7/17/13
to Justin Lebar, Kyle Huey, Andrew McCreight, dev-...@lists.mozilla.org, Firefox Dev
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

unread,
Jul 17, 2013, 1:36:29 PM7/17/13
to Justin Lebar, <dev-gaia@lists.mozilla.org>, Andrew McCreight, Firefox Dev
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
Reply all
Reply to author
Forward
0 new messages