memory leak in shadow realms

[joyeecheung]

Version

all

Platform

all

Subsystem

shadow-realm

What steps will reproduce the bug?

// Flags: --experimental-shadow-realm --max-old-space-size=20
'use strict';

for (let i = 0; i < 1000; i++) {
  const realm = new ShadowRealm();
  realm.evaluate(`new TextEncoder(); 1;`);
}

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Shouldn't crash from OOM

What do you see instead?

Crash from OOM

Additional information

See analysis #47339 (comment). This was introduced in #46809. The current memory management of shadow realms requires that there should be no strong global references in the graph, but we currently have many of them in the code base - among them are the binding data and the aliased arrays associated with the encoding binding, which is lazily created when TextEncoder is accessed, for example.

I think we have at least two options:

1. Change the memory management of shadow realms so that it doesn't rely on the context being GC'ed to GC itself (otherwise any out-of-band global reference to objects in that context holds the context alive forever) - I am not sure how feasible it is, superficially that seems plausible, because the shadow realms boundary ensures that no objects can be leaked out of the shadow realms, therefore once the shadow realms wrappers are unreachable the context should be unreachable, but it might get more complex once there's asynchronicity/modules involved
2. Avoid making BaseObjects strong - there are strong BaseObjects everywhere in the code base, however, I am not sure if it's actually practical to simply eliminate all strong global references - we might end up having some of them just impossible to make weak, and they'll result in a leak once they end up in a shadow realm.

Or, we could use some help from V8 to get notified about the realms being unreachable, and release the realms in some callback instead of in a weak callback of the context. It's not clear to me what's enough for us at this point, though.

[joyeecheung]

@nodejs/realm

[anonrig]

Could this memory leak cause performance regression when we run URL tests? I'm seeing a different result in node benchmark compare results, compared to microbenchmarks with mitata.

[mcollina]

I think we would eventually need some way to "know" when a ShadowRealm can be collected / closed / destroyed to dispose of any connected libuv handles or other native resources.

[joyeecheung]

Could this memory leak cause performance regression when we run URL tests? I'm seeing a different result in node benchmark compare results, compared to microbenchmarks with mitata.

I don't think so, this is only relevant in shadow realms, and I don't think we have URLs benchmarks in shadow realms (it's only supported starting from two weeks ago and not even released, and still it's behind --experimental-shadow-realms). Principal realms are unaffected because...you only get one principal realm per Node.js instance, and we actively destroy them when there are no more tasks to run.

[legendecas]

ShadowRealm is currently behind the runtime flag --experimental-shadow-realm or --harmony-shadow-realm and it should not affect performance in the principal realm.

I think we would eventually need some way to "know" when a ShadowRealm can be collected / closed / destroyed to dispose of any connected libuv handles or other native resources.

If BaseObjects are weak and no active HandleWraps or ReqWraps, the ShadowRealms' context should be unreachable and can be GC-ed. However, as pointed out by @joyeecheung as option 2, this needs to avoid making BaseObjects strong. This can be particularly a problem when we need to nest BaseObjects with c++ member fields by v8 global handles as v8's garbage collector is unable to infer the references between c++ objects. As an alternative, the references can be saved as object internal fields, or as v8's cppgc TracedReference.

[joyeecheung]

I also noticed that the current machinery for managing BaseObjects does not work well with the idea that "a context will become unreachable before the BaseObjects are destructed". For example I am trying to make BindingData weak, and I notice the they get deleted first via the cleanup hooks (as part of the destruction of the Realm) and then via the weak callbacks, whereas in the principal realm we simply assume that weak callbacks, if ever called, must always be called before the cleanup hooks (and we already have guard in the code for double-deletion in this case, but not the other one). There are probably more assumption about this throughout the code base. This also means, IIUC, none of the BaseObjects destructors should access the context or even modify JS states (not even setting the internal fields, that also crashes when the context is unreachable). This can be particularly problematic for complex objects like AsyncWraps (who...calls destroy hooks during destruction. Ouch).

[joyeecheung]

I now wonder if it's too late to make the ShadowRealms API actively dispose themselves (e.g. realm.dispose()) to avoid the leak. That would be the easiest solution, implementation-wise, to solve this problem. Probably harder TC39-process wise though (or it might not even be on the table - I am not sure if this constraint is just from our implementation).

[mcollina]

I now wonder if it's too late to make the ShadowRealms API actively dispose themselves (e.g. realm.dispose()) to avoid the leak. That would be the easiest solution, implementation-wise, to solve this problem. Probably harder TC39-process wise though (or it might not even be on the table - I am not sure if this constraint is just from our implementation).

From my point of view, it would be tough for us to deal with any native/asynchronous resources without a way to dispose of the realm deliberately. With the current semantics, if we open a socket inside a ShadowRealm, and the wrapper object is collected, what should we do? There are three options:

1. keep the realm going until there are native resources allocated to it
2. shut all the associated native resources down
3. crash badly

With a dispose() function, we could decide what behavior we want to put in place.

[joyeecheung]

if we open a socket inside a ShadowRealm

I think the ability of opening a socket is probably out of the scope in ShadowRealms ;). But even in those cases, those wrappers are usually weak so would not hold the context alive. The more problematic ones are e.g. caches (e.g. for module imports and serialization, which should be available in shadow realms?), we may need strong references to them to guarantee idempotence.

The AsyncWrap problem is a bit separate - they can be made weak, but also we need to make sure that they don't access the context when the weak callbacks are called, which is unsafe if the weak callbacks are called as a result of the context itself becoming reachable. This isn't possible for principal realms, because their the context is always destructed after all BaseObjects are cleaned up and async tasks are finished, but currently shadow realms have a different order. And AsyncWrap are still used in some APIs that might be available to shadow realms (e.g. anything that uses the thread pool for async work, like compression streams and crypto jobs).

[legendecas]

Yeah, modules are supposed to be supported in the shadow realm and can be imported with shadowRealm.importValue() or dynamic imports in evaluate().

I think modules are a bit different from AsyncWrap problem. IMO, we should keep the realm alive if there are any ongoing async tasks. This avoids the problem that a shadow realm is becoming unreachable before an async task is finished.