Bug 143333 – [Streams API] Collecting a ReadableStreamReader should not unlock its stream

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

[youenn fablet]

Currently ReadableStreamReader may be collected by the JS engine, in which case its related stream, if any, is automatically unlocked.
Either the stream should remain locked or the reader should not be collected if is tied to a stream.

[youenn fablet]

Created attachment 250055 [details]
Patch

[Build Bot]

Comment on attachment 250055 [details]
Patch

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

New failing tests:
streams/readablestreamreader-constructor.html

[Build Bot]

Created attachment 250056 [details]
Archive of layout-test-results from ews103 for mac-mavericks

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

[youenn fablet]

Created attachment 250059 [details]
Patch

[youenn fablet]

Comment on attachment 250059 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +

Alternative to storing a boolean would be to make ReadableStreamReader CustomIsReachable and return true if it has a stream.
But I guess the stream and reader would not be collectable anymore for the whole session, hence the current approach.

[Darin Adler]

Comment on attachment 250059 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStream.h:63
> +    void release(bool keepLocking = false) { m_reader = nullptr; m_isLocked = keepLocking; }

This should be two different named functions rather than a single function with a boolean argument. Nobody is calling this and passing a boolean expression, so treating this as an argument just makes it confusing at the call site for no reason.

Maybe the new one could be named releaseAndLeaveLocked() or releaseButKeepLocked()?

[Benjamin Poulain]

Comment on attachment 250059 [details]
Patch

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

Good patch. Nice to see coverage for the edge cases.

Anything interesting if the Stream is collected with a live reader?

>> Source/WebCore/ChangeLog:11
>> +
> 
> Alternative to storing a boolean would be to make ReadableStreamReader CustomIsReachable and return true if it has a stream.
> But I guess the stream and reader would not be collectable anymore for the whole session, hence the current approach.

Yep, that looks like a good approach. No reason to keep both objects alive.

> Source/WebCore/Modules/streams/ReadableStream.h:61
> +    bool isLocked() { return m_isLocked; }

This should be const.

>> Source/WebCore/Modules/streams/ReadableStream.h:63
>> +    void release(bool keepLocking = false) { m_reader = nullptr; m_isLocked = keepLocking; }
> 
> This should be two different named functions rather than a single function with a boolean argument. Nobody is calling this and passing a boolean expression, so treating this as an argument just makes it confusing at the call site for no reason.
> 
> Maybe the new one could be named releaseAndLeaveLocked() or releaseButKeepLocked()?

+1 for having 2 names.

Can you please also move the inline implementation after the definition of the class? It will be a little cleaner than having 2 assignment directly in the class declaration.

[youenn fablet]

(In reply to comment #7)
> Comment on attachment 250059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250059&action=review
> 
> Good patch. Nice to see coverage for the edge cases.
> 
> Anything interesting if the Stream is collected with a live reader?

That may be a usual case.
ReadableStream destructor will be called once reader will be destroyed.

> >> Source/WebCore/ChangeLog:11
> >> +
> > 
> > Alternative to storing a boolean would be to make ReadableStreamReader CustomIsReachable and return true if it has a stream.
> > But I guess the stream and reader would not be collectable anymore for the whole session, hence the current approach.
> 
> Yep, that looks like a good approach. No reason to keep both objects alive.
> 
> > Source/WebCore/Modules/streams/ReadableStream.h:61
> > +    bool isLocked() { return m_isLocked; }
> 
> This should be const.

OK

> >> Source/WebCore/Modules/streams/ReadableStream.h:63
> >> +    void release(bool keepLocking = false) { m_reader = nullptr; m_isLocked = keepLocking; }
> > 
> > This should be two different named functions rather than a single function with a boolean argument. Nobody is calling this and passing a boolean expression, so treating this as an argument just makes it confusing at the call site for no reason.
> > 
> > Maybe the new one could be named releaseAndLeaveLocked() or releaseButKeepLocked()?
> 
> +1 for having 2 names.

OK

> Can you please also move the inline implementation after the definition of
> the class? It will be a little cleaner than having 2 assignment directly in
> the class declaration.

OK

[youenn fablet]

Created attachment 250128 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 250128 [details]:

http/tests/xmlhttprequest/workers/methods-async.html bug 143403 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 250128 [details]
Patch for landing

Clearing flags on attachment: 250128

Committed r182344: <http://trac.webkit.org/changeset/182344>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.