Bug 143333 - [Streams API] Collecting a ReadableStreamReader should not unlock its stream
Summary: [Streams API] Collecting a ReadableStreamReader should not unlock its stream
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-02 05:30 PDT by youenn fablet
Modified: 2015-04-04 10:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.34 KB, patch)
2015-04-03 03:15 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (532.91 KB, application/zip)
2015-04-03 03:48 PDT, Build Bot
no flags Details
Patch (7.35 KB, patch)
2015-04-03 04:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (7.62 KB, patch)
2015-04-04 09:18 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-04-02 05:30:30 PDT
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.
Comment 1 youenn fablet 2015-04-03 03:15:19 PDT
Created attachment 250055 [details]
Patch
Comment 2 Build Bot 2015-04-03 03:48:11 PDT
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
Comment 3 Build Bot 2015-04-03 03:48:15 PDT
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
Comment 4 youenn fablet 2015-04-03 04:35:52 PDT
Created attachment 250059 [details]
Patch
Comment 5 youenn fablet 2015-04-03 05:49:13 PDT
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.
Comment 6 Darin Adler 2015-04-03 13:38:31 PDT
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()?
Comment 7 Benjamin Poulain 2015-04-03 16:42:26 PDT
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.
Comment 8 youenn fablet 2015-04-04 09:05:32 PDT
(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
Comment 9 youenn fablet 2015-04-04 09:18:01 PDT
Created attachment 250128 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2015-04-04 10:15:44 PDT
The commit-queue encountered the following flaky tests while processing attachment 250128 [details]:

http/tests/xmlhttprequest/workers/methods-async.html bug 143403 (authors: atwilson@chromium.org and levin@chromium.org)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2015-04-04 10:17:13 PDT
Comment on attachment 250128 [details]
Patch for landing

Clearing flags on attachment: 250128

Committed r182344: <http://trac.webkit.org/changeset/182344>
Comment 12 WebKit Commit Bot 2015-04-04 10:17:23 PDT
All reviewed patches have been landed.  Closing bug.