Bug 141650 - [Streams API] Reading ReadableStream ready and closed attributes should not always create a new promise
Summary: [Streams API] Reading ReadableStream ready and closed attributes should not a...
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: 141160
  Show dependency treegraph
 
Reported: 2015-02-16 09:21 PST by youenn fablet
Modified: 2015-02-24 06:48 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.09 KB, patch)
2015-02-17 06:14 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Storage in ReadableStream (7.33 KB, patch)
2015-02-21 11:41 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Storage as private in JSReadableStream (9.71 KB, patch)
2015-02-21 11:46 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Storage as private in JSReadableStream - trying to fix mac build (9.91 KB, patch)
2015-02-22 13:08 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Storage as private in JSReadableStream - trying to fix mac build again (10.18 KB, patch)
2015-02-22 13:45 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Storage as private in JSReadableStream - fixing mac build (10.07 KB, patch)
2015-02-23 01:48 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (10.29 KB, patch)
2015-02-23 09:47 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (10.90 KB, patch)
2015-02-24 05:57 PST, 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-02-16 09:21:28 PST
As discussed in https://bugs.webkit.org/attachment.cgi?id=246100&action=review, the current implementation of ReadableStream ready attribute leads to the creation of a new promise object each time one the ready attribute is read.
This should be fixed so that the same promise object is returned.
This also applies to the closed attribute.
Comment 1 youenn fablet 2015-02-16 09:24:52 PST
The simplest solution is probably to store a DeferredWrapper either at ReadableStream or JSReadableStream side.
Given that we try to keep ReadableStream away from DeferredWrapper, the idea would be to modify the binding generator to manage DeferredWrapper at JSXXX classes, whenever a Promise-type attribute happens in the corresponding IDL.
Comment 2 youenn fablet 2015-02-17 06:14:42 PST
Created attachment 246739 [details]
Patch
Comment 3 youenn fablet 2015-02-17 06:15:50 PST
(In reply to comment #2)
> Created attachment 246739 [details]
> Patch

Patch stores promise as JSReadableStream slots and adds a new DeferredWrapper constructor to use that stored promise.
Comment 4 youenn fablet 2015-02-21 11:41:14 PST
Created attachment 247053 [details]
Storage in ReadableStream
Comment 5 youenn fablet 2015-02-21 11:46:18 PST
Created attachment 247054 [details]
Storage as private in JSReadableStream
Comment 6 youenn fablet 2015-02-22 13:08:48 PST
Created attachment 247084 [details]
Storage as private in JSReadableStream - trying to fix mac build
Comment 7 youenn fablet 2015-02-22 13:45:35 PST
Created attachment 247085 [details]
Storage as private in JSReadableStream - trying to fix mac build again
Comment 8 youenn fablet 2015-02-23 01:48:26 PST
Created attachment 247107 [details]
Storage as private in JSReadableStream - fixing mac build
Comment 9 youenn fablet 2015-02-23 07:03:40 PST
(In reply to comment #8)
> Created attachment 247107 [details]
> Storage as private in JSReadableStream - fixing mac build

This patch stores readyPromise and closedPromise as internal slots of JSReadableStream. This is similar to what is described in https://streams.spec.whatwg.org/#rs-internal-slots.
Internal slots are also used in the last patch of bug 141160 to store a reference of the JSReadableStream object directly in the enqueue, close and error JS functions passed as parameters to the start and pull callbacks.

An alternative to internal slots is to store DeferredWrapper objects in ReadableStream (corresponding patch is https://bugs.webkit.org/attachment.cgi?id=247053&action=review).
I have a preference to the first approach.

In a longer term, if this pattern happens in other APIs, it may be best to make the promise objects members of the JSReadableStream class.
Comment 10 Benjamin Poulain 2015-02-23 09:34:40 PST
Comment on attachment 247107 [details]
Storage as private in JSReadableStream - fixing mac build

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

> LayoutTests/streams/readablestream-constructor.html:69
> +    assert_array_equals(Object.keys(rs), ['closed', 'ready']);

It would be interesting to test Object.getOwnPropertyNames() as well. Object.keys() only gives you enumerable properties.
Comment 11 youenn fablet 2015-02-23 09:47:06 PST
Created attachment 247125 [details]
Patch
Comment 12 Benjamin Poulain 2015-02-23 12:01:43 PST
Comment on attachment 247125 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Stored ready and closed JSPromiseDeferred as slots in JSReadableStream.
> +        Added new constructor for DeferredWrapper to use these stored values.
> +        Updated ready and closed methods to use that new constructor.
> +        Changes are covered by new test in streams/readablestream-constructor.html.

I am not a big fan of private names but I won't fight you on that. Your tests verify the correctness of this, we can change the implementation later if needed.

What I would like is a complete description in the change log. Explain the problem, two possible solutions, and give the rationale for picking private names above internal states on ReadableStream. This kind of information will be useful to anyone who wants to refactor this code in the future.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:60
> +        return jsUndefined();

I think an invalid JSValue() would be semantically better here. A private slot could contained undefined.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:66
> +void setInternalSlotToObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&, JSC::JSValue);
> +JSC::JSValue getInternalSlotFromObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&);

Isn't that a weird place to put this?

Why not just make them static in JSReadableStreamCustom.cpp?
Comment 13 youenn fablet 2015-02-23 14:58:41 PST
Thanks for the review.

(In reply to comment #12)
> Comment on attachment 247125 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247125&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Stored ready and closed JSPromiseDeferred as slots in JSReadableStream.
> > +        Added new constructor for DeferredWrapper to use these stored values.
> > +        Updated ready and closed methods to use that new constructor.
> > +        Changes are covered by new test in streams/readablestream-constructor.html.
> 
> I am not a big fan of private names but I won't fight you on that. Your
> tests verify the correctness of this, we can change the implementation later
> if needed.
> 

The current patch centralizes most things to refactor in JSReadableStreamCustom, making future refactoring a bit easier.

> What I would like is a complete description in the change log. Explain the
> problem, two possible solutions, and give the rationale for picking private
> names above internal states on ReadableStream. This kind of information will
> be useful to anyone who wants to refactor this code in the future.

Sounds good

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:60
> > +        return jsUndefined();
> 
> I think an invalid JSValue() would be semantically better here. A private
> slot could contained undefined.

OK

> 
> > Source/WebCore/bindings/js/ReadableStreamJSSource.h:66
> > +void setInternalSlotToObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&, JSC::JSValue);
> > +JSC::JSValue getInternalSlotFromObject(JSC::ExecState*, JSC::JSValue, JSC::PrivateName&);
> 
> Isn't that a weird place to put this?
> 
> Why not just make them static in JSReadableStreamCustom.cpp?

These functions were first developped for ReadableStreamJSSource and will be used for subsequent patches (https://bugs.webkit.org/attachment.cgi?id=247116&action=review for example).

Ideally these helper functions should go in some other places as they are not tied to ReadableStream. But I did not found any obvious place for them.
Comment 14 youenn fablet 2015-02-24 05:57:11 PST
Created attachment 247229 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2015-02-24 06:48:51 PST
Comment on attachment 247229 [details]
Patch for landing

Clearing flags on attachment: 247229

Committed r180559: <http://trac.webkit.org/changeset/180559>
Comment 16 WebKit Commit Bot 2015-02-24 06:48:56 PST
All reviewed patches have been landed.  Closing bug.