1542660 - Async/generator functions can keep more scope and objects than normal function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Mathieu Hofman]

User Agent: Mozilla/5.0 (X11; CrOS x86_64 11895.47.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.58 Safari/537.36

Steps to reproduce:

In an async function

• create an object
• create and save a closure that doesn't capture any of the context (doesn't reference the object)

See repro, run in the JS Shell: https://gist.github.com/mhofman/386fb3f641a0e7b23491da8647d42fe2

Actual results:

Memory leaked.
Equivalent non-async function doesn't leak.
Assigning undefined to the variable before leaving the async function prevents the leak.

Expected results:

No memory leak.
Is the engine internally creating another reference to the async function context?

Comment 1

[Jon Coppeard (:jonco)]

Thanks for the detailed bug report.

The GC itself doesn't treat async functions any differently to non-async functions so setting NI from arai who knows about how async functions are implemented.

Comment 2

[Tooru Fujisawa [:arai]]

this also applies to local variable in generator (internally they're same thing).

Comment 3

[Tooru Fujisawa [:arai]]

here's what happening:

1. savedCallback variable in the lexical environment at the top level holds reference to the arrow function (we call this A) passed to createWeakMap
2. A holds the reference to the lexical environment (we call this L) in the async function, via fun_environment slot [1]
3. object variable in L holds the reference to the object (we call this O) with id property
4. given O is alive, the weak map entry isn't removed

then, the reason why this happens only with async/generator is that, we optimize out the lexical scope in sync non-generator function [2],
and in that case A holds the reference to (I think) global's environment.

so, this specific case should be fixed once bug 1412202 gets fixed,
but still problematic when the optimization cannot be applied (for example, if there's eval or with)

[1] https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/js/src/vm/JSFunction.cpp#782
[2] bug 1412202 for applying the optimization also to generator

Comment 4

[Tooru Fujisawa [:arai]]

related code:
https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/js/src/frontend/SharedContext.h#582-592

the optimization is done based on the function's return value (when false, we can optimize), and is true for async and generator.

Comment 5

[Mathieu Hofman]

Thanks so much for the detailed explanation.

I figured the closure / arrow function was holding onto the lexical scope, I just didn't understand why it behaved differently in sync and async or generator functions (I had a hunch that would be the case for generators but didn't bother testing).

There are common pitfalls with sync functions and closures causing leaks (e.g. if multiple closures reference the same lexical scope), I was just surprised here since I knew sync functions didn't leak with a simple arrow function like that.

Comment 6

[Mathieu Hofman]

Btw, I just checked that V8 does not leak in the same manner.

Comment 7

[Liz Henry (:lizzard)]

Is there any way to tell how widely used this feature is (so how many people might hit the leak?)

Comment 8

[Tooru Fujisawa [:arai]]

technically, this is not a leak. just that there's reference.
and we apply some optimization to other case, so that there's difference between how many objects are kept, between this case (async, generator) and other (sync non-generator function).
so, this is "nice to fix", but nothing like critical issue.

Comment 10

[Jason Orendorff [:jorendorff]]

Some work I'm doing in bug 1412202 fixes this.

Comment 11

[Jason Orendorff [:jorendorff]]

The optimization in bug 1412202 causes local variables to be stored on the stack when they are not captured, so they're more transient and the GC can collect more stuff.

There would still be a leak if the user puts hundreds of local variables in the generator or async function, because we don't attempt that optimization if it would cause yield to be slow (due to copying too many local variables from the stack into the suspended frame). I guess we will just live with that. For code written by humans, things are improved.