Layering: Check for the correct owner in DetachArrayBuffer

[littledan]

This patch gives ArrayBuffers an "owner" internal slot. This slot is
used to throw a TypeError on inappropriate DetachArrayBuffer calls.

WebAssembly needs it to be an error to detach an ArrayBuffer (e.g.,
through postMessage) for ArrayBuffers which come from
WebAssembly.Memory objects. It should only be the Memory grow
operation which ever detaches this ArrayBuffer.

Corresponding WebAssembly patch: WebAssembly/spec#712

Closes #1024

[ljharb]

Is it strange to read the internal slot but never have anything that explicitly sets it to a non-undefined value?

[ljharb]

What kind of value is this? “Is not” seems very vague. It’d be ideal to specify an algorithm for comparing these values (if the type of them can’t be tightly specified), like SameValue.

[littledan]

The owner will always come from some other specification, and be a constant. I'd be fine to switch to SameValue, but I don't see what it adds, what ambiguity it resolves.

[ljharb]

It reduces how much context i need to read another spec to have.

[domenic]

The specification already uses is and SameValue fairly interchangeably. I'd suggest a separate issue for that (although I kind of remember there being one already).

[ljharb]

In cases where the types are known, yes, but they’re not specified here.

I see from the other PR that it’s a string; of this PR asserts somehow that the slot is a string, then I’m comfortable with “is” phrasing.

[littledan]

Is it strange to read the internal slot but never have anything that explicitly sets it to a non-undefined value?

I dunno, another way would be to have an abstract algorithm to set the slot, but have "nobody" call it (just have it called outside of the specification). @rossberg suggested this originally, but I went this way to be a little terser. Any preferences?

[ljharb]

I prefer having the abstract operation; I’d be interested to hear others’ thoughts.

[domenic]

I think we have precedent for the version in this PR, with [[IsHTMLDDA]]. It seems good to me.

[allenwb]

Design seems much too closely tied to a single use case and a single specific host extension. Generalize it.

Why not make it an ECMAScript language valued internal slot [[DetatchKey]] and provide an abstraction operation to set it?

Also, I'm uncomfortable that hooks like this are moving away from the ES6 design goals of self-hostable/virtualizable built-ins. There is nothing that prevents implementations from doing this already. If it is a useful enough use case to incorporated into the ES specification then we should think about how to expose the same functionality to ES app/library developers.

[domenic]

Why not make it an ECMAScript language valued internal slot [[DetatchKey]] and provide an abstraction operation to set it?

As far as I can tell that is the same as this PR, except with an unnecessary wrapper abstract op and a different name.

we should think about how to expose the same functionality to ES app/library developers.

Agreed; that is my motivation behind trying to resurrect ArrayBuffer.transfer, BTW. (Haven't had time though :(.)

I don't think that future works blocks this PR, though.

[allenwb]

As far as I can tell that is the same as this PR, except with an unnecessary wrapper abstract op and a different name.

Names are important for communicating intent. I have no idea what an "owner" is, but I think I have a pretty good intuitive understanding of what a "key" is . The wrapper method provides a place to further clarify what values are acceptable as keys.

Agreed; that is my motivation behind trying to resurrect ArrayBuffer.transfer,

I never did understand why transfer was withdrawn. It seemed like a useful userland feature even if the platform usage had gone away.

I don't think that future works blocks this PR, though.

Well, blocking this PR doesn't block WebAssembly because implementations can still do what they need to do. Given that, if blocking this PR forces TC39 to take up the work needed to make this an actual ES language level features rather than letting it float to the indefinite future, that seems like it could be a good thing.

[littledan]

I'm fine with whatever phrasing of this patch you all find the most clear. If "key" is more clear to everyone than "owner", I can do that renaming ("owner" was a suggestion from Andreas; note that there is no security content to this all). If putting things in a few abstract algorithms is clearer, I can do that as well, also add notes if this is felt to be tricky to understand.

Seems like we're not in agreement about what would be the most readable, so I'll wait a bit to hear out any more opinions before drawing a conclusion and uploading another patch which might be editorially more acceptable.

I never did understand why transfer was withdrawn. It seemed like a useful userland feature even if the platform usage had gone away.

I believe ArrayBuffer.transfer was initially proposed for growing memory for asm.js, and is now subsumed by WebAssembly's mem.grow instruction. The champions probably didn't think it was useful enough at that point.

Well, blocking this PR doesn't block WebAssembly because implementations can still do what they need to do. Given that, if blocking this PR forces TC39 to take up the work needed to make this an actual ES language level features rather than letting it float to the indefinite future, that seems like it could be a good thing.

Do you have an idea for what the language-level feature would be? I don't quite understand why the ordering has to be this way--we've added internal things first and then made language-level reifications later.

You're right that blocking this PR won't force implementations to unship. It will just prevent WebAssembly and JS from interacting in a way that conforms to the union of the three specs (Wasm, JS and the interaction spec). That seems like some technical debt that would be nice to avoid.

[bterlson]

24.1.5 needs a description of this new slot.

I agree that key is a better name than owner.

I would like to see ArrayBuffer.transfer return as well, and am sympathetic to wanting to add more spec machinery to make this more general. However, I think this PR is fine for now - let actual usage drive how much spec text we have. I don't think blocking this PR forces TC39 to do anything.

[littledan]

OK, I've changed the internal slot name to ArrayBufferDetachKey, is to SameValue, and added a comment about what this logic is for. Happy to make more changes if needed.

[ljharb]

LGTM

[allenwb]

? Do you have an idea for what the language-level feature would be? I don't quite understand why the ordering has to be this way--we've added internal things first and then made language-level reifications later.

I can't find it right now but I think I saw up thread or perhaps in a review comment that we think that the WebAssembly folk intend to use a string value as the "owner" key. If this is the case it might become a vulnerability if we decide to expose a method like ArrayBuffer.detachUsingKey(anArrayBuffer, key) and somebody leans the format of the strings used for keys used by WebAssembly. They may be able to guess use key values circumventing the non-detachment protection.. For this reason, I would expect that what we would want to use as a key would be an object or symbol value rather than a string.

[rossberg]

@allenwb, I don't think anybody suggested using a string. Clearly, the values need to be something with generative identity. But it is an internal field, so there is no need to use JavaScript values in the first place.

As for the name, I'm fine either way, though I'd note that the term "key" is rather overloaded already in the ES context.

[ljharb]

@rossberg WebAssembly/spec#712 uses a string.

[littledan]

I used a string in that spec phrasing because it seemed convenient. I did not mean that I believe there to be a good need for a language-level string owner for ArrayBuffers (or that there shouldn't be). The string is unobservable. Another option I was considering is some language around a new abstract unique token (more like what @rossberg originally suggested), but went with a string because it was simpler and got across the same thing.

I think it should be acceptable to write specification text to describe current shipping web behavior without that text being interpreted as a must-have feature to expose to JS users.

[rossberg]

@ljharb, touche. But upon reflection I tend to agree with @littledan that a string may be fine, since this is purely a spec-level device, and ultimately uniqueness relies on other potential specs playing fair. That is, even if we had a way to express allocation of unique tokens, some other spec might still come along and use the WebAssembly token. On the level of specs there isn't any enforcable encapsulation.

[allenwb]

@littledan

I don't quite understand why the ordering has to be this way--we've added internal things first and then made language-level reifications later.

@allenwb

If this is the case it might become a vulnerability if we decide to expose a method like ArrayBuffer.detachUsingKey(anArrayBuffer, key) and somebody leans the format of the strings used for keys used by WebAssembly.

@rossberg

But upon reflection I tend to agree with @littledan that a string may be fine, since this is purely a spec-level device, and ultimately uniqueness relies on other potential specs playing fair.

It seems to me, that this sequence identifies exactly the problem I'm concerned about. The new internal slot is only a specification level device until such a time as TC39 decides to provide an ES level API for accessing it. At that time, the existing implementation level usage will constrain what we can do with that API. It may even make it impossible to expose such API without impacting the integrity of the existing usage.

What is being advocated on this thread is what I characterize as "scenario-driven design" rather than system-level design. In scenario-driven design you focus on supporting a single current use case and generally ignore future use cases or future interactions with other features. At the implementation level of a system this is often the wise and pragmatic thing to do. It is "agile". It may result in internal warts, inconsistencies, and dead-ends, within the code, but those are generally invisible outside of the code level and can be fixed by future refactoring. But scenario-driven design is a terrible way to approach language design. The JS language is filled with warts, inconsistencies, and dead-end features from scenario-driven design decisions made early in its life (and some latter ones too). Language design is not something a good place to be agile.

I implore everyone working on the ES spec. to avoid "scenario-driven design". Always be a system-level designer. Don't just think about today's issue. Put in the extra effort to try to think about other use case, future requirements, and potential feature interactions.

[littledan]

This design is already more general than I was initially imagining specifying it. I was imagining that we'd have a "detach" abstract algorithm and a "force detach" abstract algorithm, and just a boolean to indicate whether this is owned or not. But @rossberg suggested a more general design, based on the detach key, which allows several specifications to interact without accidentally giving themselves the freedom to free each others' buffers. That's the design we're discussing here.

It seems that this PR is somewhat controversial, so I'll put it on the agenda for the next meeting and we can discuss it in committee to come to a conclusion.

[ljharb]

(rebased)

[littledan]

This patch got the consensus of the committee in the March 2018 TC39 meeting. The change does not result in any observable changes, so I think it is good to land without tests.

[bterlson]

I am sympathetic to Allen's concerns and will not be merging this in to 2018 (even though technically a layering change would be ok at this point?). That should hopefully give us some time to uncover a more systems-level design alternative. Feel free to open a PR with any suggested improvements along those lines.

[littledan]

Thanks for merging. I agree that this change doesn't need a backport to ES2018; the WebAssembly JS API spec links to the current draft specification anyway.