Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

postMessage, WebAssembly.Memory, and undetachable ArrayBuffers #4601

Closed
ajklein opened this issue May 8, 2019 · 11 comments · Fixed by #4603
Closed

postMessage, WebAssembly.Memory, and undetachable ArrayBuffers #4601

ajklein opened this issue May 8, 2019 · 11 comments · Fixed by #4603

Comments

@ajklein
Copy link
Contributor

ajklein commented May 8, 2019

There's a difference in behavior between Firefox and Chrome for the following code:

postMessage('foo', '*', [new WebAssembly.Memory({initial: 4}).buffer]);

Firefox throws a TypeError, while Chrome happily "transfers" the buffer. Except it doesn't transfer, but rather is silently copied. This is because the ArrayBuffer objects returned from the buffer getter are not allowed to be detached, except when the WebAssembly.Memory is grown (there are more details here, of course, but I don't think they're relevant to HTML).

It seems that neither of these behaviors is specified, e.g., there's no check about "non-detachability" or "wasm memory association" in the postMessage transfer algorithm.

The infrastructure for handling it is already available: when WebAssembly [creates a WebAssembly.Memory](https://webassembly.github.io/spec/js-api/index.html#create-a-memory-buffer] object, it sets [[ArrayBufferDetachKey]] to "WebAssembly.Memory". And ECMAScript allows passing a key to DetachArrayBuffer.

So, having looked at all that, I think this may be a trivial fix: since HTML does not pass a key argument to DetachArrayBuffer, it defaults to undefined. And therefore step 4 of DetachArrayBuffer will throw a TypeError, since SameValue("WebAssembly.Memory", undefined) is false.

Therefore, I think the fix is to replace ! with ? preceding the call to DetachArrayBuffer in step 5.4.4 of the transfer algorithm (2.8.7), and DetachArrayBuffer itself will do the right thing.

All of this is assuming the Firefox behavior is what we want, of course.

@Ms2ger @littledan @domenic @dtig @binji

@domenic
Copy link
Member

domenic commented May 8, 2019

Thanks for the detailed issue report!

I'm guessing some of the folks CCed are the ones who would be involved in determining for Chrome whether or not to adopt the Firefox behavior. Do you personally believe the Firefox behavior is better? Also, do you know who to ping from Safari?

Therefore, I think the fix is to replace ! with ? preceding the call to DetachArrayBuffer in step 5.4.4 of the transfer algorithm (2.8.7), and DetachArrayBuffer itself will do the right thing.

If the fix is indeed this simple, I think we should also add a note explaining why it might throw, as this is pretty subtle :).

@ajklein
Copy link
Contributor Author

ajklein commented May 8, 2019

I prefer the Firefox behavior, but Chrome has been shipping its copy-instead-of-transfer behavior for long enough that I'd want to do some investigation before changing it in a stable release.

@kmiller68 is the main Safari person who I've been interacting with around wasm stuff.

@binji
Copy link

binji commented May 8, 2019

Yeah, the current Chrome behavior is possible to implement (by copying yourself), and is also surprising. But it also seems like the kind of thing that websites could be relying on. Do we have any UMA stats for when this happens? If not, can we add some?

@ajklein
Copy link
Contributor Author

ajklein commented May 8, 2019

After some digging in the code, I think this behavior goes all the way back to 2014 (before WebAssembly, this behavior was present for asm.js), and isn't currently UMA-counted. We can add a counter, but given how long we've had this behavior in the wild it may be hard to change.

@binji
Copy link

binji commented May 8, 2019

How many ways can you create a undetachable ArrayBuffer? (just wasm and asm.js?)

@ajklein
Copy link
Contributor Author

ajklein commented May 8, 2019

I'm actually pretty fuzzy on the asm.js case: it seems from code inspection that at that time, in V8, it was true of asm.js memories, but those are no longer special.

@littledan
Copy link
Contributor

Thanks for filing this issue. The Firefox semantics sound ideal to me, if they are web-compatible. This was an unintentional omission of mine when formalizing the WebAssembly/JS interface; I was misunderstanding that the serialize algorithm would handle this.

@kmiller68
Copy link

Safari also throws a TypeError if you try to transfer a WebAssembly.Memory generated ArrayBuffer. I agree with others that throwing seems like the ideal semantics.

Ms2ger added a commit that referenced this issue May 9, 2019
In particular, DetachArrayBuffer() can throw for the ArrayBuffer used as the
backing memory in the WebAssembly JS API.

Fixes #4601.
Ms2ger added a commit that referenced this issue May 9, 2019
In particular, DetachArrayBuffer() can throw for the ArrayBuffer used as the
backing memory in the WebAssembly JS API.

This matches the current behavior in Firefox and Safari.

Fixes #4601.
@littledan
Copy link
Contributor

littledan commented May 9, 2019

Note, I added this throwing behavior in tc39/ecma262#1112 + WebAssembly/spec#712 . We discussed it in the March 2018 TC39 meeting. The only real oversight seems to be ? vs ! (and the lack of tests!), as #4603 fixes.

Ms2ger added a commit that referenced this issue May 9, 2019
In particular, DetachArrayBuffer() can throw for the ArrayBuffer used as the
backing memory in the WebAssembly JS API.

This matches the current behavior in Firefox and Safari.

Fixes #4601.
domenic pushed a commit that referenced this issue May 9, 2019
In particular, DetachArrayBuffer() can throw for the ArrayBuffer used as the
backing memory in the WebAssembly JS API.

This matches the current behavior in Firefox and Safari.

Fixes #4601.
@annevk
Copy link
Member

annevk commented May 10, 2019

I filed whatwg/webidl#724 and WebAudio/web-audio-api#1861 as follow-ups.

@domenic
Copy link
Member

domenic commented May 10, 2019

Also whatwg/streams#1001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants