Repository navigation
Pin how stream constructors read source, sink and transformer members - #7784
Conversation
|
✅ Approved — No issues found. The view-validation consolidation keeps the existing checks and moves the WebIDL conversion ahead of the state checks, which matches the spec's order. The new tests cover the changed paths. The decision is advisory: this comment never approves, blocks or requests changes on the pull request. ✅ All checks passed🔒 Security: ✅ LGTMThe SharedArrayBuffer rejection and the fixed-length buffer transfer at the byte-stream boundary still happen. Metadata is still read only through the captured intrinsic getters. 📖 AGENTS.md: ✅ LGTMThe 📋 Code Quality: ✅ LGTM
The pull request drops the separate "backing buffer has zero byteLength" checks. The new comment explains why they cannot fail: the byteLength getters return 0 for a detached or out-of-bounds typed array, and a DataView getter throws in that case. That reasoning holds.
⚡ Performance: ✅ LGTMNo performance concerns. Passing the captured 📝 Documentation: ✅ LGTMThe new comments describe the current behavior. In the Streams spec's WebIDL, Reviewed commit: |
|
Since last review: 0 resolved, 0 still open, 0 new. Reviewed commit: c556a530 · github run |
43c56b3 to
cff4d79
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
LGTM, two non-blocking notes inline.
DUP-22 step 1, test only. read(view), readAtLeast(), enqueue() and respondWithNewView() take an ArrayBufferView in WebIDL. No test passed them anything else: read(view) pinned only the error class, and the other two nothing, so a wrong message from their type checks would pass every test. nonViewArgumentsRejected pins a string, an object and undefined at all four, and a bare ArrayBuffer. Every non-view is a TypeError in both implementations, with their own messages, except a bare ArrayBuffer at enqueue() and respondWithNewView(): C++ types those parameters as a BufferSource and takes the buffer as a view over the whole of it, detaching it and delivering its bytes. TypeScript rejects it, as the spec does. Readable-byte ledger #41 records the divergence.
DUP-22 step 2. read(view) and enqueue() ran the same view checks, twelve lines apart only in the noun, and all three byte-stream trust boundaries (with respondWithNewView()) checked for an ArrayBufferView before calling getUnsharedViewInfo. getUnsharedViewInfo now makes that check itself, which completes WebIDL's ArrayBufferView conversion in one place, and the new getNonEmptyUnsharedViewInfo adds the non-empty checks that read(view) and enqueue() share. The messages are unchanged. The comment on validateAndTransferView named respondWithNewView(), which has never called it; it now names enqueue() alone.
DUP-22 step 3. byobRequest.respondWithNewView() read the view's metadata through getUnsharedViewInfo for its own checks, then handed the controller the view, which read it again through getViewInfo. No code runs in between, so the second read could only repeat the first. The controller now takes the ViewInfo the request read, which saves the read and its allocation on every call. Nothing observable changes.
DUP-22 step 4. read(view) and enqueue() checked, after a view's own byteLength, that its buffer's byteLength was not zero, as the spec's second step does. That check could never fail: the byteLength getters read 0 for a detached or out-of-bounds typed array, a DataView's throw, and a view with bytes lies within its buffer. The spec needs the step because a view's [[ByteLength]] slot keeps its value once its buffer is detached. A probe of seventeen kinds of input (non-views, shared, zero-length, detached, out-of-bounds and emptied length-tracking views) at all five entry points never reached it. The check goes, with its message, and a comment says why the spec's step needs no code. This saves a getter call per enqueue() and read(view). Nothing observable changes.
DUP-22 step 5. Since respondWithNewView()'s controller takes the ViewInfo its request read, getViewInfo has had one caller, getUnsharedViewInfo, which makes the isArrayBufferView check that getViewInfo's precondition asked of its callers. getViewInfo folds into it, and the precondition goes. The getters run in the same order. Nothing observable changes.
DUP-22 step 6. WebIDL converts a method's arguments before its steps
run, and C++ converts its parameters before the method body.
respondWithNewView() checked that its request was still valid, and
enqueue() that its stream could still take a chunk, before converting
the view, so a value that was not a view met those checks first:
respondWithNewView('x') on an invalidated request threw 'This BYOB
request has been invalidated', and enqueue('x') after close() 'Cannot
enqueue a chunk into a stream that is closed or closing', where C++
threw its parameter-type error.
Both now convert the view first. enqueue() also checks that the chunk
is not empty (spec steps 1-2) before the stream's state (steps 3-4),
as C++ does, and transfers the buffer only once both pass, so
validateAndTransferView, which did both at once, goes. A view backed
by a SharedArrayBuffer, which the conversion rejects, now fails as
such on an invalidated request too, where C++, which converts it,
still reports the invalidated request. Every error is still a
TypeError; only which message wins changes.
viewConvertedBeforeMethodSteps pins the new order on both
implementations, and fails under TypeScript without the change.
b7cb7d3 to
c556a53
Compare
2 participants
Deduplicate and test view argument validation. See individual commits for details