Bug 227690 – ReadableStream's pipeTo() and pipeThrough() don't handle options in spec-perfect way

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Shvayka]

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.

[Radar WebKit Bug Importer]

<rdar://problem/80482144>

[youenn fablet]

Created attachment 434748 [details]
Patch

[EWS Watchlist]

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

[Alexey Shvayka]

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.

[youenn fablet]

Created attachment 434777 [details]
Patch

[youenn fablet]

Created attachment 434779 [details]
Patch

[youenn fablet]

https://github.com/youennf/wpt/tree/wpt-export-for-webkit-227690

[youenn fablet]

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.

[youenn fablet]

Comment on attachment 434779 [details]
Patch

bot error is unrelated

[Alexey Shvayka]

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).

[youenn fablet]

Created attachment 434833 [details]
Patch for landing

[youenn fablet]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/29880

[youenn fablet]

Thanks for the review, I updated the change log and reduced the try/catch as you suggested.

[EWS]

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].