Bug 215267

Created attachment 406170 [details] Patch

<rdar://problem/66680142>

Created attachment 406172 [details] Patch

Created attachment 406271 [details] Patch

Created attachment 406272 [details] Patch

Created attachment 406288 [details] Patch

Created attachment 406290 [details] Patch

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?

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

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.

(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?

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

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

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.

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.

Created attachment 406452 [details] Patch for landing

Committed r265548: <https://trac.webkit.org/changeset/265548> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406452 [details].