Refresh WritableStream up to spec

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Refresh WritableStream up to spec

[youenn fablet]

Created attachment 406170 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/66680142>

[youenn fablet]

Created attachment 406172 [details]
Patch

[youenn fablet]

Created attachment 406271 [details]
Patch

[youenn fablet]

Created attachment 406272 [details]
Patch

[youenn fablet]

Created attachment 406288 [details]
Patch

[youenn fablet]

Created attachment 406290 [details]
Patch

[Sam Weinig]

Comment on attachment 406290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406290&action=review

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-by-pipe.any-expected.txt:3
> -PASS using pipeTo on Response body should disturb it synchronously 
> -PASS using pipeThrough on Response body should disturb it synchronously 
> +FAIL using pipeTo on Response body should disturb it synchronously |this| is not a Promise
> +FAIL using pipeThrough on Response body should disturb it synchronously |this| is not a Promise

This seems like a regression. Why is ok to start failing this?

[youenn fablet]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 406290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406290&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-by-pipe.any-expected.txt:3
> > -PASS using pipeTo on Response body should disturb it synchronously 
> > -PASS using pipeThrough on Response body should disturb it synchronously 
> > +FAIL using pipeTo on Response body should disturb it synchronously |this| is not a Promise
> > +FAIL using pipeThrough on Response body should disturb it synchronously |this| is not a Promise
> 
> This seems like a regression. Why is ok to start failing this?

The current pipeTo/pipeThrough implementation is using an old version of WritableStream API. WritableStream API surface changed and the pipe algorithm changed as well.

Note that the current pipeTo/pipeThrough implementation works with any object that mimics the old WritableSteam API.
That is why I plan to update the pipeTo algorithm soon, but still keep the current algorithm (that works with any JS object following the old API surface) for a few weeks.

[youenn fablet]

Note also that this patch does not change the current ReadableStream implementation.
This patch should have no behavioural effects if WritableStream is disabled, which is the default value.

[Sam Weinig]

(In reply to youenn fablet from comment #9)
> (In reply to Sam Weinig from comment #8)
> > Comment on attachment 406290 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=406290&action=review
> > 
> > > LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-stream-disturbed-by-pipe.any-expected.txt:3
> > > -PASS using pipeTo on Response body should disturb it synchronously 
> > > -PASS using pipeThrough on Response body should disturb it synchronously 
> > > +FAIL using pipeTo on Response body should disturb it synchronously |this| is not a Promise
> > > +FAIL using pipeThrough on Response body should disturb it synchronously |this| is not a Promise
> > 
> > This seems like a regression. Why is ok to start failing this?
> 
> The current pipeTo/pipeThrough implementation is using an old version of
> WritableStream API. WritableStream API surface changed and the pipe
> algorithm changed as well.
> 
> Note that the current pipeTo/pipeThrough implementation works with any
> object that mimics the old WritableSteam API.
> That is why I plan to update the pipeTo algorithm soon, but still keep the
> current algorithm (that works with any JS object following the old API
> surface) for a few weeks.

Sorry, I'm not following. Why does this change regress this test?

[youenn fablet]

> Sorry, I'm not following. Why does this change regress this test?

This patch is changing WritableStream API surface.
ReadableStream.pipeTo implementation is using the WritableStream API so we are breaking this implementation hence the test failures since the tests use WritableStream.

ReadableStream.pipeTo needs to be updated according the new algorithm defined by the spec, that will no longer make use of WritableStream public API but lower level private APIs.
Once pipeTo implementation is updated, the tests should go back to PASS.

[Sam Weinig]

Is there a way to make this change without breaking the existing functionality?

[Geoffrey Garen]

Comment on attachment 406290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406290&action=review

r=me

> Source/WebCore/Modules/streams/WritableStream.js:59
> +    // FIXME: underlyingSink, underlyingSink

Not sure what this FIXME means

> Source/WebCore/Modules/streams/WritableStreamInternals.js:170
> +    for (let index = 0, length = requests.length; index < length; ++index)
> +        requests[index].@reject.@call(@undefined, storedError);

Is it intentional that we'll get an exception if writeRequests shrinks during this iteration?

> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:22
> -PASS ReadableStream (two chunks enqueued, then closed): piping with no options and no destination errors 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: false } and no destination errors 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and no destination errors 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: false } and a destination with that errors synchronously 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and a destination with that errors synchronously 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and a destination that errors on the last chunk 
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with no options and no destination errors |this| is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: false } and no destination errors |this| is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and no destination errors |this| is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: false } and a destination with that errors synchronously |this| is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and a destination with that errors synchronously |this| is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and a destination that errors on the last chunk |this| is not a Promise

What's up with these newly introduced test failures? Not catastrophic, since this is off by default -- but seems like a significant bug. Can we make 'this' a Promise? Same comment for the other test failures.

[youenn fablet]

Thanks for the review.

(In reply to Geoffrey Garen from comment #14)
> Comment on attachment 406290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406290&action=review
> 
> r=me
> 
> > Source/WebCore/Modules/streams/WritableStream.js:59
> > +    // FIXME: underlyingSink, underlyingSink
> 
> Not sure what this FIXME means

Will comment that we should have probably more WebIDL type checks.

> > Source/WebCore/Modules/streams/WritableStreamInternals.js:170
> > +    for (let index = 0, length = requests.length; index < length; ++index)
> > +        requests[index].@reject.@call(@undefined, storedError);
> 
> Is it intentional that we'll get an exception if writeRequests shrinks
> during this iteration?

This cannot happen, @reject might execute some code but it is guaranteed to be exeuted asynchronously.


> > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:22
> > -PASS ReadableStream (two chunks enqueued, then closed): piping with no options and no destination errors 
> > -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: false } and no destination errors 
> > -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and no destination errors 
> > -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: false } and a destination with that errors synchronously 
> > -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and a destination with that errors synchronously 
> > -PASS ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and a destination that errors on the last chunk 
> > +FAIL ReadableStream (two chunks enqueued, then closed): piping with no options and no destination errors |this| is not a Promise
> > +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: false } and no destination errors |this| is not a Promise
> > +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and no destination errors |this| is not a Promise
> > +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: false } and a destination with that errors synchronously |this| is not a Promise
> > +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and a destination with that errors synchronously |this| is not a Promise
> > +FAIL ReadableStream (two chunks enqueued, then closed): piping with { preventClose: true } and a destination that errors on the last chunk |this| is not a Promise
> 
> What's up with these newly introduced test failures? Not catastrophic, since
> this is off by default -- but seems like a significant bug. Can we make
> 'this' a Promise? Same comment for the other test failures.

A follow-up patch will change ReadableStream.pipeTo in case WritableStream flag is defined.

[youenn fablet]

Created attachment 406452 [details]
Patch for landing

[EWS]

Committed r265548: <https://trac.webkit.org/changeset/265548>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406452 [details].