Bug 159871 – [Streams API] ReadableStreamController methods should throw if its stream is not readable

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

As per the spec, close and enqueue are aligned with error method. They should throw if stream is not readable.

[youenn fablet]

Created attachment 283889 [details]
Patch

[Build Bot]

Comment on attachment 283889 [details]
Patch

Attachment 283889 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1700405

New failing tests:
streams/reference-implementation/readable-stream-templated.html
streams/reference-implementation/pipe-to-options.html

[Build Bot]

Created attachment 283892 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 283889 [details]
Patch

Attachment 283889 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1700407

New failing tests:
streams/reference-implementation/readable-stream-templated.html
streams/reference-implementation/pipe-to-options.html

[Build Bot]

Created attachment 283893 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[youenn fablet]

Created attachment 283984 [details]
Patch

[youenn fablet]

(In reply to comment #4)
> Comment on attachment 283889 [details]
> Patch
> 
> Attachment 283889 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/1700407
> 
> New failing tests:
> streams/reference-implementation/readable-stream-templated.html
> streams/reference-implementation/pipe-to-options.html

Fixed failures for these tests.
Note that pipeTo is still in flux.
These tests should probably be tightened/updated once pipeTo is finalised.

[Xabier Rodríguez Calvar]

Comment on attachment 283984 [details]
Patch

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

> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:6
> +CONSOLE MESSAGE: line 405: TypeError: ReadableStream is not readable
> +CONSOLE MESSAGE: line 405: TypeError: ReadableStream is not readable
> +CONSOLE MESSAGE: line 406: TypeError: ReadableStream is not readable
> +CONSOLE MESSAGE: line 406: TypeError: ReadableStream is not readable
> +
> +Harness Error (FAIL), message = TypeError: ReadableStream is not readable

Why does this happen? Can you point the affected tests?

[youenn fablet]

(In reply to comment #8)
> Comment on attachment 283984 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283984&action=review
> 
> > LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:6
> > +CONSOLE MESSAGE: line 405: TypeError: ReadableStream is not readable
> > +CONSOLE MESSAGE: line 405: TypeError: ReadableStream is not readable
> > +CONSOLE MESSAGE: line 406: TypeError: ReadableStream is not readable
> > +CONSOLE MESSAGE: line 406: TypeError: ReadableStream is not readable
> > +
> > +Harness Error (FAIL), message = TypeError: ReadableStream is not readable
> 
> Why does this happen? Can you point the affected tests?

This happens twice for the same templated test that tries to asynchronously enqueue and then close the stream (two chunks enqueued async, then closed).

But pipeTo already closed the stream in two cases, hence why enqueue and close are throwing for those configurations.
I would probably be nicer to add some assert_throw but I am not sure this is worth it.
These tests with these console messages exercice well whether the stream is closed or not.

[Xabier Rodríguez Calvar]

Comment on attachment 283984 [details]
Patch

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

>>> LayoutTests/streams/reference-implementation/readable-stream-templated-expected.txt:6
>>> +Harness Error (FAIL), message = TypeError: ReadableStream is not readable
>> 
>> Why does this happen? Can you point the affected tests?
> 
> This happens twice for the same templated test that tries to asynchronously enqueue and then close the stream (two chunks enqueued async, then closed).
> 
> But pipeTo already closed the stream in two cases, hence why enqueue and close are throwing for those configurations.
> I would probably be nicer to add some assert_throw but I am not sure this is worth it.
> These tests with these console messages exercice well whether the stream is closed or not.

Sorry, I repent the r+ :), the problem with this output is that you don't know which test is causing that output, I think that needs to be changed.

[youenn fablet]

I'll update the test according what repo test then.
Might be good to move these to W3C WPT repo also.
Do you remember why they were not migrated?

[Xabier Rodríguez Calvar]

(In reply to comment #11)
> I'll update the test according what repo test then.

Good.

> Might be good to move these to W3C WPT repo also.

It would be good.

> Do you remember why they were not migrated?

IIRC, Domenic only migrated some tests and if some of them were not finished it was either because of lack of time (I think he began with "stable" RS features and left WR and friends out) or because things were not stable enough.

[youenn fablet]

Created attachment 284013 [details]
Rebasing tests according whatwg

[Xabier Rodríguez Calvar]

Comment on attachment 284013 [details]
Rebasing tests according whatwg

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

> LayoutTests/streams/reference-implementation/readable-stream-templated.html:407
> +                if (!this._cancelled)
> +                    c.enqueue(chunks[0]);

Though it might be interesting to check that the exception is being thrown here, this is checked in other places so it is not strictly necessary. Feel free to add it if you feel like.

[youenn fablet]

(In reply to comment #14)
> Comment on attachment 284013 [details]
> Rebasing tests according whatwg
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284013&action=review
> 
> > LayoutTests/streams/reference-implementation/readable-stream-templated.html:407
> > +                if (!this._cancelled)
> > +                    c.enqueue(chunks[0]);
> 
> Though it might be interesting to check that the exception is being thrown
> here, this is checked in other places so it is not strictly necessary. Feel
> free to add it if you feel like.

Thanks for the review.
I prefer keeping close to the original tests here.

[WebKit Commit Bot]

Comment on attachment 284013 [details]
Rebasing tests according whatwg

Clearing flags on attachment: 284013

Committed r203411: <http://trac.webkit.org/changeset/203411>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.