Bug 143608 – [Streams API] Implement ReadableStreamController

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Implement ReadableStreamController as defined in https://streams.spec.whatwg.org/#rs-controller-class

[youenn fablet]

Created attachment 250603 [details]
Patch

[youenn fablet]

Created attachment 250605 [details]
Trying to fix mac build

[Xabier Rodríguez Calvar]

Comment on attachment 250605 [details]
Trying to fix mac build

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

> Source/WebCore/ChangeLog:1
> [phone]  Youenn Fablet  <[e-mail]>

Do not forget me ;)

> Source/WebCore/Modules/streams/ReadableStreamController.h:44
> +/* This class is only used for JS source readable streams to allow enqueuing, closing or erroring a readable stream.
> +   Its definition is at https://streams.spec.whatwg.org/#rs-controller-class.
> +   Note that its constructor is taking a ReadableJSStream as it should only be used for JS sources.*/

// Comments

> LayoutTests/ChangeLog:1
> [phone]  Youenn Fablet  <[e-mail]>

Me.

> LayoutTests/streams/reference-implementation/readable-stream.html:78
> -
> -            assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'constructor', 'enqueue', 'error' ]);
> +            // FIXME: decide whether exposing the constructor.
> +            assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'enqueue', 'error' ]);

I think we should do what the reference implementation does, which is exposing the constructor. Therefore this wouldn't be needed.

> LayoutTests/streams/reference-implementation/readable-stream.html:94
> -            assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> -            assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> -            assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> +            assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'length', 'name' ]);
> +            assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'length', 'name' ]);
> +            assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'length', 'name' ]);
>  
> -            assert_equals(enqueue.name, '');
> -            assert_equals(close.name, '');
> -            assert_equals(error.name, '');
> +            assert_equals(enqueue.name, 'enqueue');
> +            assert_equals(close.name, 'close');
> +            assert_equals(error.name, 'error');

This test has changed. It needs to be rebased. Also, I don't think we should be removing the things that are part of spec tests, we should comply.

> LayoutTests/streams/reference-implementation/readable-stream.html:115
> -            assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'constructor', 'enqueue', 'error' ]);
> +            assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'enqueue', 'error' ]);
>              controller.test = "";
> -            assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'constructor', 'enqueue', 'error' ]);
> +            assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'enqueue', 'error' ]);

Ditto.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 250605 [details]
> Trying to fix mac build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250605&action=review
> 
> > Source/WebCore/ChangeLog:1
> > [phone]  Youenn Fablet  <[e-mail]>
> 
> Do not forget me ;)
Woups, sure.

> > LayoutTests/streams/reference-implementation/readable-stream.html:78
> > -
> > -            assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'constructor', 'enqueue', 'error' ]);
> > +            // FIXME: decide whether exposing the constructor.
> > +            assert_array_equals(Object.getOwnPropertyNames(Object.getPrototypeOf(controller)).sort(), [ 'close', 'enqueue', 'error' ]);
> 
> I think we should do what the reference implementation does, which is
> exposing the constructor. Therefore this wouldn't be needed.

The binding generator is not supporting NoInterfaceObject and CustomConstructor, hence the issue.
The constructor is always throwing an exception, hence not all that useful.
This could be fixed in JSReadablestreamCustom.cpp later on.

> > LayoutTests/streams/reference-implementation/readable-stream.html:94
> > -            assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> > -            assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> > -            assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> > +            assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'length', 'name' ]);
> > +            assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'length', 'name' ]);
> > +            assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'length', 'name' ]);
> >  
> > -            assert_equals(enqueue.name, '');
> > -            assert_equals(close.name, '');
> > -            assert_equals(error.name, '');
> > +            assert_equals(enqueue.name, 'enqueue');
> > +            assert_equals(close.name, 'close');
> > +            assert_equals(error.name, 'error');
> 
> This test has changed. It needs to be rebased. Also, I don't think we should
> be removing the things that are part of spec tests, we should comply.

With testharness tests, the test stops after the first failed assertion.
In that case, we are exiting the test without having tested any API really.
By updating the test, we still test all implemented properties.

I think we can update the tests once implementing the API.
When finalizing, we can resync all tests.

[youenn fablet]

Created attachment 250705 [details]
Rebased patch

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> > > LayoutTests/streams/reference-implementation/readable-stream.html:94
> > > -            assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> > > -            assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> > > -            assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'arguments', 'caller', 'length', 'name', 'prototype' ]);
> > > +            assert_array_equals(Object.getOwnPropertyNames(enqueue).sort(), [ 'length', 'name' ]);
> > > +            assert_array_equals(Object.getOwnPropertyNames(close).sort(), [ 'length', 'name' ]);
> > > +            assert_array_equals(Object.getOwnPropertyNames(error).sort(), [ 'length', 'name' ]);
> > >  
> > > -            assert_equals(enqueue.name, '');
> > > -            assert_equals(close.name, '');
> > > -            assert_equals(error.name, '');
> > > +            assert_equals(enqueue.name, 'enqueue');
> > > +            assert_equals(close.name, 'close');
> > > +            assert_equals(error.name, 'error');
> > 
> > This test has changed. It needs to be rebased. Also, I don't think we should
> > be removing the things that are part of spec tests, we should comply.
> 
> With testharness tests, the test stops after the first failed assertion.
> In that case, we are exiting the test without having tested any API really.
> By updating the test, we still test all implemented properties.
> 
> I think we can update the tests once implementing the API.
> When finalizing, we can resync all tests.

Commenting with Youenn on IRC we have two options here in order to not lose track of what we have now and what is the spec goal:

1. We could have in a comment which is the real goal of the test and keep running what we have now.
2. Keep the spec test failing and have a custom test that would be removed once  the everything is implemented and the spec test passes.

I was thinking that 1 would be better but maybe 2 is better if we refer to the goal spec test in the description of the custom one.

[Build Bot]

Comment on attachment 250705 [details]
Rebased patch

Attachment 250705 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4539333881102336

New failing tests:
streams/readable-stream.html

[Build Bot]

Created attachment 250707 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250705 [details]
Rebased patch

Attachment 250705 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5976024936349696

New failing tests:
streams/readable-stream.html

[Build Bot]

Created attachment 250708 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Xabier Rodríguez Calvar]

(In reply to comment #7)
> Comment on attachment 250705 [details]
> Rebased patch
> 
> Attachment 250705 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/4539333881102336
> 
> New failing tests:
> streams/readable-stream.html

In this case, as tests are not in the spec yet, so I think we can update our custom test.

[Benjamin Poulain]

Comment on attachment 250705 [details]
Rebased patch

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

Was this supposed to be for review?

I think this is going in the right direction. I don't think this covers the cases I mentioned in the other bug.

> Source/WebCore/Modules/streams/ReadableStreamController.h:60
> +    ReadableJSStream* m_stream;

Please explain the lifetime of the objects in the Changelog.

> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:46
> +    if (impl().stream().internalState() != ReadableStream::State::Readable) 

I would store impl().stream() in a temporary variable.

> LayoutTests/streams/reference-implementation/brand-checks-expected.txt:9
> +FAIL ReadableStreamController enforces a brand check on its argument assert_throws: Constructing a ReadableStreamController should throw function "function () { new ReadableStreamController(fakeReadableSt..." did not throw

Looks like we have a regression here.

[youenn fablet]

(In reply to comment #12)
> Comment on attachment 250705 [details]
> Rebased patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250705&action=review
> 
> Was this supposed to be for review?
> 
> I think this is going in the right direction. I don't think this covers the
> cases I mentioned in the other bug.
> 
> > Source/WebCore/Modules/streams/ReadableStreamController.h:60
> > +    ReadableJSStream* m_stream;
> 
> Please explain the lifetime of the objects in the Changelog.

Sounds good.

> 
> > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:46
> > +    if (impl().stream().internalState() != ReadableStream::State::Readable) 
> 
> I would store impl().stream() in a temporary variable.

OK

> > LayoutTests/streams/reference-implementation/brand-checks-expected.txt:9
> > +FAIL ReadableStreamController enforces a brand check on its argument assert_throws: Constructing a ReadableStreamController should throw function "function () { new ReadableStreamController(fakeReadableSt..." did not throw
> 
> Looks like we have a regression here.

As long as the controller constructor is not implemented, all corresponding tests are not really relevant.
I will upload a new patch without controller constructor.
I do not really see the point of adding support for it but I will upload another patch dedicated to the constructor so that we can continue discussing that specific point.

[youenn fablet]

Created attachment 250784 [details]
Rebasing webkit specifc test

[youenn fablet]

> I do not really see the point of adding support for it but I will upload
> another patch dedicated to the constructor so that we can continue
> discussing that specific point.

Patch is available at https://bugs.webkit.org/show_bug.cgi?id=143752, on top of this latest patch.

[youenn fablet]

Comment on attachment 250784 [details]
Rebasing webkit specifc test

Patch is rebased according current trunk.
Compared to previous patch, it does not implement close() functionality.
Plan is to add support for it in 143363 after this one is shipped, which is probably more logical that way.

[Benjamin Poulain]

Comment on attachment 250784 [details]
Rebasing webkit specifc test

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

> Source/WebCore/ChangeLog:12
> +        A controller is created at the time a ReadableJSStream is started and it is owned by it.
> +        The controller will be destroyed when the ReadableJSStream is also destroyed.

Sorry I did not have the time to review any stream patch today. :(

Quick question: The ReadableStreamController has a wrapper that keep a reference to it. Can't the wrapper outlive the ReadableJSStream?

[youenn fablet]

Created attachment 250907 [details]
Fixing wrapper issue

[youenn fablet]

(In reply to comment #17)
> Comment on attachment 250784 [details]
> Rebasing webkit specifc test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250784&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        A controller is created at the time a ReadableJSStream is started and it is owned by it.
> > +        The controller will be destroyed when the ReadableJSStream is also destroyed.
> 
> Sorry I did not have the time to review any stream patch today. :(
> 
> Quick question: The ReadableStreamController has a wrapper that keep a
> reference to it. Can't the wrapper outlive the ReadableJSStream?

Good spot!
Fixed the issue and added a related async test case.
I also filed https://bugs.webkit.org/show_bug.cgi?id=143818 as the implementation of ReadableStreamJSSource::startAsync was defeating garbage collection purpose.

[youenn fablet]

Comment on attachment 250907 [details]
Fixing wrapper issue

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

> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:52
> +    notImplemented();

Will be implemented within rebased patch of https://bugs.webkit.org/show_bug.cgi?id=143363

> LayoutTests/streams/reference-implementation/brand-checks-expected.txt:-10
> -PASS ReadableStreamController can't be given a fully-constructed ReadableStream 

These regressions are fixed in https://bugs.webkit.org/show_bug.cgi?id=143752

[Xabier Rodríguez Calvar]

Comment on attachment 250907 [details]
Fixing wrapper issue

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

> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:48
> +        return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no reaadablestream")));

Typo reaadablestream

> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:50
> +    if (stream->internalState() != ReadableStream::State::Readable) 

Trailing space at the end of the line

> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:60
> +        return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no reaadablestream")));

Typo

> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:69
> +        return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Controller has no reaadablestream")));

Typo

[Xabier Rodríguez Calvar]

Created attachment 250948 [details]
Patch

Fixed typos and trailing spaces.

[Xabier Rodríguez Calvar]

Created attachment 251229 [details]
Patch

Replaced /**/ comments

[Benjamin Poulain]

Comment on attachment 251229 [details]
Patch

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

Quick review but nothing looks out of place.

> Source/WebCore/ChangeLog:12
> +        A controller is created at the time a ReadableJSStream is started and it is owned by it.
> +        The controller will be destroyed when the ReadableJSStream is also destroyed.

Am I getting this right?
-ReadableStreamJSSource creates the ReadableStreamController
-It creates a JS wrapper for the controller.
-You keep a strong reference to the wrapper to ensure that ReadableStreamController does not outlive ReadableStreamJSSource.

[youenn fablet]

(In reply to comment #24)
> Comment on attachment 251229 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251229&action=review
> 
> Quick review but nothing looks out of place.

Thanks.

> > Source/WebCore/ChangeLog:12
> > +        A controller is created at the time a ReadableJSStream is started and it is owned by it.
> > +        The controller will be destroyed when the ReadableJSStream is also destroyed.
> 
> Am I getting this right?
> -ReadableStreamJSSource creates the ReadableStreamController
> -It creates a JS wrapper for the controller.
> -You keep a strong reference to the wrapper to ensure that
> ReadableStreamController does not outlive ReadableStreamJSSource.


That is it, maybe the change log could be further improved when landing.

As you spotted previously, ReadableStreamController may outlive ReadableStreamJSSource if the JS keeps a reference to it.
In that case, as seen in ReadableStreamJSSource destructor, ReadableStreamController stream is set to null, which makes the controller returns exception for any function call.
This could also be added in the change log.

[Xabier Rodríguez Calvar]

Created attachment 251313 [details]
Patch

Fixed changelog according to comments

[Xabier Rodríguez Calvar]

Comment on attachment 251313 [details]
Patch

Landing based on previous r+

[WebKit Commit Bot]

Comment on attachment 251313 [details]
Patch

Clearing flags on attachment: 251313

Committed r183107: <http://trac.webkit.org/changeset/183107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.