Fix DisposeCapability leak

[rbuckton]

The DisposeResources AO fails to indicate that the resources in its [[DisposableResourceStack]] slot will no longer be used and can be freed. As a result, a DisposableStack will inadvertently hold resources in memory until the DisposableStack instance is freed, which is not an intended behavior.

This changes DisposeResources to set the [[DisposableResourceStack]] to a new empty list, along with a NOTE indicating the resources can be freed.

A PR against ecma262 is being tracked by rbuckton/ecma262#8.

Fixes #191

/cc: @tc39/ecma262-editors

[github-actions]

A preview of this PR can be found at https://tc39.es/proposal-explicit-resource-management/pr/194.

[mhofman]

As a result, a DisposableStack will inadvertently hold resources in memory until the DisposableStack instance is freed, which is not an intended behavior.

Yeah technically there is no future execution which would be able to observe the instance held in [[DisposableResourceStack]] of a disposed stack, so a strict reading of the definition of liveness would make this unnecessary, but I doubt any sane implementation would check whether the stack has already been disposed when doing gc, so making this explicit is better.

[rbuckton]

I've added this for discussion in the April 2024 plenary.

[syg]

Yeah technically there is no future execution which would be able to observe the instance held in [[DisposableResourceStack]] of a disposed stack, so a strict reading of the definition of liveness would make this unnecessary, but I doubt any sane implementation would check whether the stack has already been disposed when doing gc, so making this explicit is better.

I agree with @mhofman's reading. Strictly speaking, slots already don't keep things alive in and of themselves (we don't have reachability, we have liveness via observability).

Making a note doesn't hurt and makes the spec more readable. I'd call this an editorial fix, not a normative one.

[syg]

I'd set it to an empty list to avoid having to deal with a sentinel value everywhere else.

[rbuckton]

I'd set it to an empty list to avoid having to deal with a sentinel value everywhere else.

The upside of the sentinel value is that it makes things abundantly clear that a DisposeCapability is never intended to be reused, since we can assert on the absence of a list.

[syg]

You can also assert that the list is empty, no? In fact you can assert arbitrary English sentences, this isn't a program.

[rbuckton]

Asserting the list is empty would break DisposableStack/AsyncDisposableStack. Asserting a clearly defined state avoids ambiguity for spec readers and implementers. i.e.,

Assert: This DisposeCapability has never been passed as an argument to DisposeResources.

This seems way too ambiguous on its own.

[syg]

Asserting the list is empty would break DisposableStack/AsyncDisposableStack

I don't follow, say more?

[rbuckton]

DisposableStack and AsyncDisposableStack also use DisposeCapability, which starts with an empty list. This should not cause an Assert, or even a TypeError:

const stack = new DisposableStack();
stack.dispose(); // DisposeCapability.[[DisposableResourceStack]] will be empty here.

In addition, using x = null does not add an entry to DisposeCapability.[[DisposableResourceStack]] either (only await using x = null does to ensure we trigger an Await), so an empty list is a perfectly valid state for a non-disposed DisposeCapability.

[syg]

But you don't even need these asserts if you use an empty list. AFAICT the reasoning chain goes: you want to editorially convey that the list doesn't keep things alive -> you null it out with a sentinel value -> you add asserts that the sentinel values don't flow into places it shouldn't.

I still don't see what's lost by changing the above to: you want to editorially convey that the list doesn't keep things alive -> you empty the list. You get the same editorial effect, and fewer chances of editorial errors by flowing the sentinel into wrong places.

[rbuckton]

I'm concerned about the risk that a DisposeCapability is inadvertently reused. I am fine with changing this to just reset to an empty list, however.

[littledan]

If you do want to use a sentinel value, you. might want to use something more evocative, like ~disposed~.

[littledan]

+1 to @syg and @mhofman -- this is not a leak currently. If we considered every internal slot which is left with things in it to be a memory leak, we'd have to change tons and tons of stuff in the spec. If we want to give implementations a hint, we could just note at the point where you'd set it to undefined that the contents will never be used again, and they could feel free to set it to undefined.

[rbuckton]

@syg, does the latest commit align with your suggestion?

[syg]

lgtm. Added a suggestion to call out GC specifically since that was the editorial intent.