Streams spec for pipeThrough(): https://streams.spec.whatwg.org/#rs-pipe-through WebIDL spec for dictionary types: https://heycam.github.io/webidl/#es-dictionary Implementation: https://github.com/WebKit/WebKit/blob/3cd76abcda098f91d46fb0ad42e1eab68b6ddb12/Source/WebCore/Modules/streams/ReadableStream.js#L122 > if (@isReadableStreamLocked(this)) > throw @makeTypeError("ReadableStream is locked"); While this is the first step of pipeThrough() algorithm, it should happen after all arguments (including `options`) are checked / handled. > if (options === @undefined) > options = { }; This causes properties to queried from Object.prototype; the WebIDL spec doesn't perform [[Get]] for non-objects at all. @Object.@create(null) may be used instead. > let signal; > if ("signal" in options) { > signal = options["signal"]; This is observable is options is a Proxy; the WebIDL spec merely compares [[Get]] result with undefined. > const preventClose = !!options["preventClose"]; > const preventAbort = !!options["preventAbort"]; > const preventCancel = !!options["preventCancel"]; For these 3, lookup order matches the member declaration order of Streams spec. However, the WebIDL spec requires "lexicographical order"; does it translate to 1) "preventAbort" 2) "preventCancel" 3) "preventClose"? Either way, `options.signal` should be queried after these 3, which is observable by a userland getter.
<rdar://problem/80482144>
Created attachment 434748 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment on attachment 434748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434748&action=review > Source/WebCore/Modules/streams/ReadableStream.js:133 > + if (options != @undefined) { Aha, this fixes very important issue that I've missed. Seems like before this change, a TypeError was thrown (from `in` operator) for `options` that is `null` instead of using the defaults (step 1 of https://heycam.github.io/webidl/#es-dictionary). This does fix it, yet we should rather use @isUndefinedOrNull built-in because `== @undefined` is false-positive for `document.all`. It would be nice to have `options = null` test case if we haven't already. > Source/WebCore/Modules/streams/ReadableStream.js:134 > + preventAbort = !!options["preventAbort"]; Also, we should throw here `if (!@isObject(options))` per step 1 to avoid lookups on primitives, which are observable. A test would be nice. > Source/WebCore/Modules/streams/ReadableStream.js:136 > + preventClose = !!options["preventClose"]; Since we are changing these lines, could we please use (semantically equivalent) `options.preventAbort` syntax? Square bracket access is normally used with a variable rather than string literal. > Source/WebCore/Modules/streams/ReadableStream.js:139 > + if (signal !== @undefined && !(signal instanceof @AbortSignal)) It would be nice to have `{ signal: undefined }` test case if we haven't already.
Created attachment 434777 [details] Patch
Created attachment 434779 [details] Patch
https://github.com/youennf/wpt/tree/wpt-export-for-webkit-227690
Thanks Alexey, I updated tests accordingly. > > Source/WebCore/Modules/streams/ReadableStream.js:136 > > + preventClose = !!options["preventClose"]; > > Since we are changing these lines, could we please use (semantically > equivalent) `options.preventAbort` syntax? > Square bracket access is normally used with a variable rather than string > literal. I kept the way the spec is written and the way it is done for now in other stream related implementation files. I guess it is a tad shorter so there is a small gain to change.
Comment on attachment 434779 [details] Patch bot error is unrelated
Comment on attachment 434779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434779&action=review r=me with 2 nits. Thank you for fixing this, Youenn! > Source/WebCore/ChangeLog:9 > + Make one getter for signal. Technically, `in` doesn't invoke a getter, only a Proxy trap. Apart from not invoking getters on Object.prototype and tweaking their order, this patch also fixed `options = null` case, added exception for primitive `options`, and fixed exception handling in pipeTo(). It would be nice to mention that in ChangeLog. > Source/WebCore/Modules/streams/ReadableStream.js:182 > + try { It's nice to have this fixed. A suggestion: what if we limit try { ... } scope only to rethrow userland errors, while returning rejected Promises for non-object `options` and incorrect `signal`? Like if (!@isObject(options)) return @Promise.@reject(@makeTypeError("options must be an object")); try { preventAbort = !!options["preventAbort"]; preventCancel = !!options["preventCancel"]; preventClose = !!options["preventClose"]; signal = options["signal"]; } catch(e) { return @Promise.@reject(e); } if (signal !== @undefined && !(signal instanceof @AbortSignal)) return @Promise.@reject(@makeTypeError("options.signal must be AbortSignal")); This way it's more clear that we are handling getter errors, and it's consistent with the rest of the checks like if (!@isWritableStream(destination)) etc. `instanceof` can't throw for @AbortSignal (and it will be replaced with @isAbortSignal after this patch is landed).
Created attachment 434833 [details] Patch for landing
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/29880
Thanks for the review, I updated the change log and reduced the try/catch as you suggested.
Committed r280593 (240215@main): <https://commits.webkit.org/240215@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434833 [details].