Summary: | ReadableStream's pipeTo() and pipeThrough() don't handle options in spec-perfect way | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||||||
Component: | DOM | Assignee: | youenn fablet <youennf> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, calvaris, clopez, ews-watchlist, joepeck, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://github.com/web-platform-tests/wpt/pull/29880 | ||||||||||||
Attachments: |
|
Description
Alexey Shvayka
2021-07-05 14:24:44 PDT
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
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]. |