Bug 142866 – [Streams API] Implement a barebone ReadableStreamReader interface

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

[youenn fablet]

We should implement the IDL defined at https://streams.spec.whatwg.org/#reader-class

[youenn fablet]

Created attachment 249102 [details]
Patch

[Build Bot]

Comment on attachment 249102 [details]
Patch

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

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

Created attachment 249103 [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 249102 [details]
Patch

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

New failing tests:
js/dom/global-constructors-attributes.html

[Build Bot]

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

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

[youenn fablet]

Created attachment 249105 [details]
Trying to fix expectations

[youenn fablet]

Created attachment 249123 [details]
Removing unneeded state

[youenn fablet]

Comment on attachment 249123 [details]
Removing unneeded state

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

> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:49


Should be moved to ReadableStreamReader constructor to ease ReadableStreamReader subclassing

> Source/WebCore/Modules/streams/ReadableStreamReader.h:69
> +    ReadableStream* m_stream;

Should be initialized to nullptr

> Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp:118
> +    RefPtr<ReadableStreamReader> readableStreamReader = ReadableStreamReader::create(stream->impl());

Should be replaced by stream->impl().createReader() so that stream decides which Reader subclass is used.

[youenn fablet]

Created attachment 249566 [details]
Moving suspendIfNeeded within constructor

[Benjamin Poulain]

Comment on attachment 249566 [details]
Moving suspendIfNeeded within constructor

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

Quick review. The patch does not seem to match the spec to me. Am I misreading something?

> Source/WebCore/Modules/streams/ReadableStream.h:62
> +    void setReader(ReadableStreamReader* reader) { m_reader = reader; }

Maybe the name should also carry the information that this is a set-once operation. Something like:
    lockStreamToReader(ReadableStreamReader)
    lock(ReadableStreamReader)

If I am not mistaken, the "reader" argument must be non-null. If that's the case, it should be passed by reference to the method.

> Source/WebCore/Modules/streams/ReadableStream.h:64
> +    // This method should be overriden by subclasses to create more specific stream readers.
> +    virtual Ref<ReadableStreamReader> createReader();

I would just make this virtual when needed. No point in paying this cost now.

> Source/WebCore/Modules/streams/ReadableStream.h:77
> +    RefPtr<ReadableStreamReader> m_reader;

Shouldn't the ref() be in the other direction.

You can get to ReaderStream from ReadableStreamReader, but you can't get an existing ReadableStreamReader from ReaderStream.

> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:66
> +void ReadableStreamReader::setStream(ReadableStream& stream)

Any reason this is not done by the constructor?

> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:85
> +    // FIXME: We should try and do better here.
> +    return false;

We recently discovered that ActiveDOMObject impact on the page cache is larger than expected.

We will have to be careful with this before enabling Streams by default.

> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:63
> +    if (impl().reader())
> +        return toJS(exec, globalObject(), impl().reader());
> +    return toJS(exec, globalObject(), impl().createReader());

Does this match the spec?

In the case when calling getReader() when there is already an existing reader.

How I read it is:
-https://streams.spec.whatwg.org/#rs-get-reader is  AcquireReadableStreamReader(this)
-https://streams.spec.whatwg.org/#acquire-readable-stream-reader creates a new object every time.
-The constructor of ReadableStreamReader would then fail since the Stream is locked https://streams.spec.whatwg.org/#reader-constructor

What am I missing?

[youenn fablet]

(In reply to comment #10)
> Comment on attachment 249566 [details]
> Moving suspendIfNeeded within constructor
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249566&action=review
> 
> Quick review. The patch does not seem to match the spec to me. Am I
> misreading something?

Thanks for the review.
Patch does not align to the spec and will be corrected shortly.

> > Source/WebCore/Modules/streams/ReadableStream.h:62
> > +    void setReader(ReadableStreamReader* reader) { m_reader = reader; }
> 
> Maybe the name should also carry the information that this is a set-once
> operation. Something like:
>     lockStreamToReader(ReadableStreamReader)
>     lock(ReadableStreamReader)
> 
> If I am not mistaken, the "reader" argument must be non-null. If that's the
> case, it should be passed by reference to the method.

It was planned to use setReader with nullptr to release the stream.
But it will be more readable to use more meaningful method names.
lock and release sounds ok.

> > Source/WebCore/Modules/streams/ReadableStream.h:64
> > +    // This method should be overriden by subclasses to create more specific stream readers.
> > +    virtual Ref<ReadableStreamReader> createReader();
> 
> I would just make this virtual when needed. No point in paying this cost now.

OK.
A future patch should make it virtual pure.

> > Source/WebCore/Modules/streams/ReadableStream.h:77
> > +    RefPtr<ReadableStreamReader> m_reader;
> 
> Shouldn't the ref() be in the other direction.
> 
> You can get to ReaderStream from ReadableStreamReader, but you can't get an
> existing ReadableStreamReader from ReaderStream.

Right.

> > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:66
> > +void ReadableStreamReader::setStream(ReadableStream& stream)
> 
> Any reason this is not done by the constructor?
> 
> > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:85
> > +    // FIXME: We should try and do better here.
> > +    return false;
> 
> We recently discovered that ActiveDOMObject impact on the page cache is
> larger than expected.
> 
> We will have to be careful with this before enabling Streams by default.
> 

Do you have a pointer to better understand this and the granularity we should try to reach?

> > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:63
> > +    if (impl().reader())
> > +        return toJS(exec, globalObject(), impl().reader());
> > +    return toJS(exec, globalObject(), impl().createReader());
> 
> Does this match the spec?
> 
> In the case when calling getReader() when there is already an existing
> reader.
> 
> How I read it is:
> -https://streams.spec.whatwg.org/#rs-get-reader is 
> AcquireReadableStreamReader(this)
> -https://streams.spec.whatwg.org/#acquire-readable-stream-reader creates a
> new object every time.
> -The constructor of ReadableStreamReader would then fail since the Stream is
> locked https://streams.spec.whatwg.org/#reader-constructor
> 
> What am I missing?

Nothing, you are just plain right!

[youenn fablet]

Created attachment 249730 [details]
Updated according review

[Benjamin Poulain]

Comment on attachment 249730 [details]
Updated according review

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

Everything looks good to me. Some comments:

> Source/WebCore/Modules/streams/ReadableStream.h:62
> +    void lock(ReadableStreamReader& reader) { m_reader = &reader; }

Can this ASSERT_WITH_MESSAGE(!reader())?

If I am not mistaken, it is never valid to replace the Reader on a locked stream.

> Source/WebCore/Modules/streams/ReadableStream.h:63
> +    Ref<ReadableStreamReader> createReader();

On the WebCore side, you have two ways to instantiate the Reader: ReadableStream::createReader() and ReadableStreamReader::create().

Unless you plan to use this as a factory, it would be better to get rid of ReadableStream::createReader().

> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:64
> +}

In the destructor, you should unlock the ReadableStream.

Since they are both ref-counted, some other object may hold a reference onto the ReadableStream, keeping it alive. The link ReadableStream->ReadableStreamReader must be cut here or we risk a dangling pointer.

[Benjamin Poulain]

(In reply to comment #11)
> > > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:66
> > > +void ReadableStreamReader::setStream(ReadableStream& stream)
> > 
> > Any reason this is not done by the constructor?
> > 
> > > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:85
> > > +    // FIXME: We should try and do better here.
> > > +    return false;
> > 
> > We recently discovered that ActiveDOMObject impact on the page cache is
> > larger than expected.
> > 
> > We will have to be careful with this before enabling Streams by default.
> > 
> 
> Do you have a pointer to better understand this and the granularity we
> should try to reach?

I think Closed and Errored state should have no problem being suspended?

Ready would be good but that would depend on the source. We should not keep a socket or a file open in the page cache.

[youenn fablet]

(In reply to comment #13)
> Comment on attachment 249730 [details]
> Updated according review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249730&action=review
> 
> Everything looks good to me. Some comments:
> 
> > Source/WebCore/Modules/streams/ReadableStream.h:62
> > +    void lock(ReadableStreamReader& reader) { m_reader = &reader; }
> 
> Can this ASSERT_WITH_MESSAGE(!reader())?
> 
> If I am not mistaken, it is never valid to replace the Reader on a locked
> stream.

OK
Let's add this assert in reader constructor.

> 
> > Source/WebCore/Modules/streams/ReadableStream.h:63
> > +    Ref<ReadableStreamReader> createReader();
> 
> On the WebCore side, you have two ways to instantiate the Reader:
> ReadableStream::createReader() and ReadableStreamReader::create().
> 
> Unless you plan to use this as a factory, it would be better to get rid of
> ReadableStream::createReader().

Next patch will remove ReadableStreamReader::create and make ReadableStream::createReader virtual pure.

> > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:64
> > +}
> 
> In the destructor, you should unlock the ReadableStream.
> 
> Since they are both ref-counted, some other object may hold a reference onto
> the ReadableStream, keeping it alive. The link
> ReadableStream->ReadableStreamReader must be cut here or we risk a dangling
> pointer.

I thought about this and forgot to properly fix it, thanks!
Later on, we will need to ensure that ReadableStreamReader does not get collected if it is locking a stream.

[youenn fablet]

(In reply to comment #15)
> (In reply to comment #13)
> > Comment on attachment 249730 [details]
> > Updated according review
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=249730&action=review
> > 
> > Everything looks good to me. Some comments:
> > 
> > > Source/WebCore/Modules/streams/ReadableStream.h:62
> > > +    void lock(ReadableStreamReader& reader) { m_reader = &reader; }
> > 
> > Can this ASSERT_WITH_MESSAGE(!reader())?
> > 
> > If I am not mistaken, it is never valid to replace the Reader on a locked
> > stream.
> 
> OK
> Let's add this assert in reader constructor.
> 
> > 
> > > Source/WebCore/Modules/streams/ReadableStream.h:63
> > > +    Ref<ReadableStreamReader> createReader();
> > 
> > On the WebCore side, you have two ways to instantiate the Reader:
> > ReadableStream::createReader() and ReadableStreamReader::create().
> > 
> > Unless you plan to use this as a factory, it would be better to get rid of
> > ReadableStream::createReader().
> 
> Next patch will remove ReadableStreamReader::create and make
> ReadableStream::createReader virtual pure.

Next patch meaning bug 143130

[youenn fablet]

Created attachment 249806 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 249806 [details]
Patch for landing

Clearing flags on attachment: 249806

Committed r182180: <http://trac.webkit.org/changeset/182180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.