Fix BufferSource algorithms for shared and resizable buffers - #1529
Fix BufferSource algorithms for shared and resizable buffers#1529MattiasBuelens wants to merge 37 commits into
BufferSource algorithms for shared and resizable buffers#1529Conversation
|
@annevk Could you assign reviewer(s) to this? Thanks! 🙏 |
MattiasBuelens
left a comment
There was a problem hiding this comment.
Quick self-review.
| 1. [=/Assert=]: |arrayBuffer| is an {{ArrayBuffer}} or {{SharedArrayBuffer}} object. | ||
| 1. Let |jsArrayBuffer| be the result of [=converted to a JavaScript value|converting=] | ||
| |arrayBuffer| to a JavaScript value. | ||
| 1. If [$IsDetachedBuffer$](|jsArrayBuffer|) is true, then return the empty |
There was a problem hiding this comment.
If you pass a TA backed by a detached buffer, then the Set |offset| to |bufferSource|'s [=ArrayBufferView/byte offset=]. step above will throw. It's weird that passing a TA backed by a detached buffer throws while passing the detached buffer itself returns the empty byte sequence (in this step). It doesn't fundamentally break anything, but it's inconsistent.
There was a problem hiding this comment.
You're right, we should check if bufferSource's underlying buffer is detached before we try to read bufferSource's byte offset.
I'll move this to the preceding "if bufferSource is a buffer view type instance" step.
There was a problem hiding this comment.
I've moved the detached check all the way to the top. I also check if length is zero, so reading byte offset later on won't throw.
Funnily enough, reading bufferSource's byte length itself can still throw for an out-of-bounds DataView (as you already reported)... Do we want "get a copy of the bytes held" to propagate that error? Or should we return an empty byte sequence in that case? 🤔
There was a problem hiding this comment.
I changed it so we read both byte length and byte offset before we check if length is zero. That way, any exceptions from reading those properties will be thrown first.
ce8c232 to
cf28af9
Compare
The rules of thumb here are:
|
|
AI found a number of substantive issues that appear to be correct: https://gist.github.com/annevk/123a3994ba82ec664b787559803cdabf (it's also incorrect about some things though; our Bikeshed setup would definitely fail on link errors). |
cf28af9 to
6201623
Compare
| allocated. If they are in the same [=agent cluster=], then implementations will just | ||
| change the backing pointers to get the same observable results with better performance | ||
| and no allocations. | ||
| * Sufficient memory cannot be allocated in the [=current realm=], which is the [=realm=] |
There was a problem hiding this comment.
I'm not sure if we have to fully spell out the meaning of "current realm" here? Happy to remove that.
annevk
left a comment
There was a problem hiding this comment.
This looks okay to me now, but how do we want to handle tests and such?
|
We'll need some APIs that already have
So I think the best way forward right now is to use the WebAssembly methods for testing. It'll be a bit awkward since the test inputs will have to be valid WASM modules, but I think I can make that work. I'll take a look. 🙂 |
|
I've opened web-platform-tests/wpt#62301, which tests "get a copy of the bytes held by the buffer source" across all combinations of resizable/shared buffers and fixed-length/length-tracking/out-of-bounds views. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
%TypedArray%.prototype.byteOffset returns +0 for an out-of-bounds typed array; only DataView.prototype.byteOffset throws. Mirror that split, like byte length already does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Matches how IsArrayBufferViewOutOfBounds and DataView.prototype.byteLength dispatch, and removes two asserts that only restated the branch condition. Also consolidates the three per-step equivalence notes into one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading |view|'s byte length or byte offset throws for an out-of-bounds DataView, so the existing asserts could throw. Assert the precondition first instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bind the resizability once instead of duplicating the ArrayBufferCopyAndDetach call, drop the redundant detached check (step 5 of that operation already throws), and use <emu-val>undefined</emu-val>. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The realm parameter was removed. ArrayBufferCopyAndDetach allocates with %ArrayBuffer%, so the new buffer is always allocated in the current realm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both algorithms now throw for an out-of-bounds DataView, where byte length previously returned a stale [[ByteLength]]. Call that out with algorithm-level notes, like the existing detach and transfer notes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Match the 33 existing plain Assert: steps; converting the whole file to [=/Assert=] can be a separate editorial change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Matches how BufferSource/detached is referenced throughout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Get a copy of the bytes" returns the empty byte sequence for a detached buffer source before it ever reads the byte length, so only a non-detached out-of-bounds DataView reaches the throwing path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This reverts commit 0c786ab.
3fff79f to
9a418e6
Compare
|
I ran the new tests from web-platform-tests/wpt#62301 against stable Chromium and Firefox, and there were already a few issues. 😅 Chromium currently rejects However, when Chromium decides to fix this bug, we'll need to decide what behavior they should be implementing for
Note that I haven't tested WebKit yet, since I don't have a macOS device. It might be interesting to know how they handle out-of-bounds @bakkot Are we still happy with this change as discussed earlier? If yes, I'll make sure to get implementer's interest from the Gecko folks, since this will change observable behavior in their implementation. |
|
I'll review this in a day or two though I'm somewhat rusty on the webidl side of things. FWIW, another possible fix for the DataView issue is to make those operations not throw on DataViews in ECMAScript (and then have the web match). Making errors into non-errors is almost always web-compat. (Fun fact: ArrayBuffer.prototype.byteLength used to be spec'd to throw also; this was changed in tc39/ecma262#2164.) |
Interesting! Looks like this was raised before: tc39/ecma262#2635. I'll comment there to bring this back to TC39's attention. |
Previously, many
BufferSourcealgorithms were using internal slots such as[[ByteOffset]]and[[ByteLength]]directly. However, with the addition ofSharedArrayBuffer(#353, #1311) and[AllowResizable](#982), this comes with extra caveats:SharedArrayBuffer, we need to useArrayBufferByteLengthto matchSharedArrayBuffer.prototype.byteLength.TypedArrayByteLength. Similarly, for a length-trackingDataView, we need to useGetViewByteLength.DataView, we first need to check whether the view is still in bounds usingIsArrayBufferViewOutOfBounds."Byte length" and "byte offset" both mirror JavaScript's asymmetry between typed arrays and
DataViews: an out-of-bounds typed array reports 0, whereas an out-of-boundsDataViewthrows aTypeError. That way, implementations can reuse their existing getters.When reading the length of a buffer, we use
SEQ-CSTordering to match ECMAScript. When reading the contents of a buffer, we keep usingUNORDERED, as "get a copy of the bytes held by the buffer source" already did. See #1529 (comment).Transferring an
ArrayBuffermust also take into account whether it should remain resizable or not. Fortunately, we can use the newArrayBufferCopyAndDetachoperation for that.preserveResizabilityparameter to "transfer anArrayBuffer", so specifications can choose whether to preserve resizability or not. By default, this is not preserved. I intend to use it in Streams, see Add explainer for resizable buffers for BYOB readers streams#1360.realmparameter of that algorithm, sinceArrayBufferCopyAndDetachdoesn't accept such a parameter. According to WebDex, no specs were actually using that parameter anyway.Normative changes, which need a review of existing callers:
TypeErrorfor aDataViewthat is detached or out of bounds, where it previously returned a stale[[ByteLength]]. This is reachable without[AllowResizable], simply by detaching the underlying buffer. "Get a copy of the bytes held by the buffer source" inherits this. Both algorithms have a note documenting it, but every existing caller is currently written as an infallible read.realmparameter. Per WebDex this is not a breaking change in practice, but it does change the exported signature.ArrayBuffernow yields a fixed-lengthArrayBufferby default, instead of reusing the original buffer's resizability.Relates to #1312.
Fixes #1385.
validate()andcompile()web-platform-tests/wpt#62301(See WHATWG Working Mode: Changes for more details.)
Preview | Diff