Bug 150627 - [Streams API] Rework promises to use @newPromiseCapability
Summary: [Streams API] Rework promises to use @newPromiseCapability
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords:
Depends on:
Blocks: 150835
  Show dependency treegraph
 
Reported: 2015-10-28 09:20 PDT by Xabier Rodríguez Calvar
Modified: 2015-11-03 06:53 PST (History)
5 users (show)

See Also:


Attachments
Patch (22.82 KB, patch)
2015-10-28 09:34 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (30.70 KB, patch)
2015-10-29 12:13 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (15.33 KB, patch)
2015-10-30 07:52 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (15.58 KB, patch)
2015-11-03 01:51 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xabier Rodríguez Calvar 2015-10-28 09:20:55 PDT
[Streams API] Rework promises to use @newPromiseCapabilites and as much InternalPromise as possible
Comment 1 Xabier Rodríguez Calvar 2015-10-28 09:34:46 PDT
Created attachment 264216 [details]
Patch

We can't use InternalPromise everywhere because if we or the user mix them with any other promises, they should have the Promise constructor, otherwise the chain of promise resolution can be broken and bad things can happen (as my tests showed). InternalPromise can be used only when we are not going to leave the outside world see them.
Comment 2 youenn fablet 2015-10-28 23:11:56 PDT
Comment on attachment 264216 [details]
Patch

The streams API implementation looks great with the proposed changes!

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

> Source/WebCore/ChangeLog:12
> +        the outside world or mix with it, meaning that can't resolv them against something coming from the used because

s/resolv/resolve
s/used/user

As an unrelated note, we probably never want any @InternalPromise to reach web page JavaScript code.
I wonder whether we want or can assert that in some ways, like in debug mode.

> Source/JavaScriptCore/builtins/Operations.Promise.js:85
> +    // FIXME: Check isConstructor(constructor).

Do we need constructor parameter?
Is there a use case for newResolvedPromiseCapability(@InternalPromise);?

If we keep newResolvedPromiseCapability like it is, can we share more code between newResolvedPromiseCapability, newRejectedPromiseCapability and newPromiseCapability?
Comment 3 Xabier Rodríguez Calvar 2015-10-29 01:31:49 PDT
(In reply to comment #2)
> > Source/WebCore/ChangeLog:12
> > +        the outside world or mix with it, meaning that can't resolv them against something coming from the used because
> 
> s/resolv/resolve
> s/used/user

Yep, good catch. Can I fix this during landing?

> As an unrelated note, we probably never want any @InternalPromise to reach
> web page JavaScript code.
> I wonder whether we want or can assert that in some ways, like in debug mode.

Fixing the generators, probably, to ensure the constructor is always Promise.

> > Source/JavaScriptCore/builtins/Operations.Promise.js:85
> > +    // FIXME: Check isConstructor(constructor).
> 
> Do we need constructor parameter?
> Is there a use case for newResolvedPromiseCapability(@InternalPromise);?
> 
> If we keep newResolvedPromiseCapability like it is, can we share more code
> between newResolvedPromiseCapability, newRejectedPromiseCapability and
> newPromiseCapability?

The idea of adding the constructor was to make newResolvedPromiseCapability and newRejectedPromiseCapability fully compatible with newPromiseCapability and the latter uses the constructor to be able to build Promise and @InternalPromise. So yes, the more shared code, the better, IMHO.
Comment 4 youenn fablet 2015-10-29 01:57:37 PDT
Reading a bit more, I think @newResolvedPromise might not fix everything.
Maybe we should add a test checking that even if a web page tries to observe promise built-ins handling by modifying Promise, it is not happening.
Or patch could be split in two maybe?
Comment 5 Yusuke Suzuki 2015-10-29 02:58:54 PDT
Comment on attachment 264216 [details]
Patch

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

> Source/WebCore/Modules/streams/WritableStream.js:147
> +    return promise.@promise;

Does it accidentally expose @InternalPromise?
Comment 6 Xabier Rodríguez Calvar 2015-10-29 03:09:30 PDT
(In reply to comment #5)
> > Source/WebCore/Modules/streams/WritableStream.js:147
> > +    return promise.@promise;
> 
> Does it accidentally expose @InternalPromise?

It does, very good catch!
Comment 7 Yusuke Suzuki 2015-10-29 04:05:40 PDT
Comment on attachment 264216 [details]
Patch

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

:) And added notes about InternalPromise.

>> Source/WebCore/ChangeLog:12
>> +        the outside world or mix with it, meaning that can't resolv them against something coming from the used because
> 
> s/resolv/resolve
> s/used/user
> 
> As an unrelated note, we probably never want any @InternalPromise to reach web page JavaScript code.
> I wonder whether we want or can assert that in some ways, like in debug mode.

Right. @InternalPromise should not be exposed to users.
See the comment in JSInternalPromise.h.
Comment 8 Xabier Rodríguez Calvar 2015-10-29 12:13:08 PDT
Created attachment 264331 [details]
Patch

Fixed the leak of @InternalPromise in write. Used @Promise to shield the code against the user changing the Promise constructor or its prototype. I kept the constructor parameter to the functions to create the vended capabilities for coherence. I think cabling @Promise or Promise there is a bad idea.
Comment 9 youenn fablet 2015-10-29 16:17:04 PDT
A test would be good to ensure this is doing the right thing.
For instance, are we sure @Promise cannot be accessed from returned promise objects?

I am not sure we should add the new promise resolved/rejected routines. They can probably be replaced by 1-liner code directly in streams api code. That way, no need to touch JSGlobalObject.
Comment 10 Xabier Rodríguez Calvar 2015-10-30 07:52:33 PDT
Created attachment 264396 [details]
Patch
Comment 11 Xabier Rodríguez Calvar 2015-10-30 08:07:45 PDT
(In reply to comment #9)
> A test would be good to ensure this is doing the right thing.
> For instance, are we sure @Promise cannot be accessed from returned promise
> objects?

After doing more thorough checks, I was able to access and modify thru getting the constructor from the promise both by adding either @Promise or Promise and disrupting the behavior.

It is not possible either to use anything like @Promise.@resolve because those functions are not declared as such internal slots so they can't be used.

I don't see any solution so shield the code from interferences from outside.

Besides this, the spec speaks about creating promises according to the promise spec. I understand that it might be interesting to shield the Streams code from users changing their behavior, but considering that it is JS and objects have prototypes and you can change those from JS, I see also reasons to keep it like this and if the user changes the prototype enough to disrupt the promises behavior, then they can get trouble and should accept it.

> I am not sure we should add the new promise resolved/rejected routines. They
> can probably be replaced by 1-liner code directly in streams api code. That
> way, no need to touch JSGlobalObject.

I did a oneliner implementation it in this case. The issue is that we are keeping promise capabilities instead of promises so we have to create them like that and that implies creating an object with the @promise and the empty @resolve and @reject functions. In this case, we can replace those with oneliners, but I don't think it pays off because it is more error prone (forgetting to set the @reject or @resolve) and you avoid all checks, which are ok, IMHO. That oneliner is more complicated, perform less checks and is more error prone, so I think we should keep the functions.
Comment 12 youenn fablet 2015-10-30 12:24:42 PDT
Comment on attachment 264396 [details]
Patch

LGTM.

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

> Source/WebCore/ChangeLog:14
> +        user, because of the same reason.

Remove this sentence may make sense since the latest patch does not add/remove any @InternalPromise use.
Renaming the bug title to "[Streams API] Rework promises to use @newPromiseCapability" might be nice as well?

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:51
> +        this.@closedPromise = { @promise: Promise.resolve(undefined), @resolve: function() {}, @reject: function() {} };

We should not need @resolve nor @reject fields.
Can you try removing them and see whether that works as is?
If that requires additional changes, let's do that as a follow-up patch.

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:57
> +    this.@closedPromise = { @promise: Promise.reject(stream.@storedError), @resolve: function() {}, @reject: function() {} };

Ditto.

> Source/WebCore/Modules/streams/WritableStream.js:46
> +    this.@readyPromise = { @promise: Promise.resolve(undefined), @resolve: function() {}, @reject: function() {} };

Ditto.

> Source/WebCore/Modules/streams/WritableStream.js:58
> +    this.@startedPromise = { @promise: Promise.resolve(startResult), @resolve: function() {}, @reject: function() {} };

Ditto.

Also, in that case, we do not need to use a @promise wrapper.
I guess this is done for consistency with other fields.
If code related to startedPromise remains the same or is kept at the same place, removing the wrapper makes sense to me.

Also, the current @XXPromise names are a bit error prone since they no longer store promises directly.
We could use @XXPromiseCapability but this is a bit long.
Any idea?

> Source/WebCore/Modules/streams/WritableStream.js:137
> +    const promise = @newPromiseCapability(Promise);

This is an example of the above issue.

> Source/WebCore/Modules/streams/WritableStream.js:148
> +    return promise.@promise;

Ditto.
Comment 13 youenn fablet 2015-10-30 12:36:00 PDT
As of improving the implementation shield, I did not dig into the issue yet.
Here are some thoughts that might be useful (or not!).

One step is to retrieve the Promise constructor object safely.
Using directly "Promise" is probably not safe.
"@Promise" seems to refer to a different Promise constructor (not sure of its purpose).
Maybe we can change it so as to get the regular Promise constructor safely?

Based on it, we can implement resolve and reject using code like:
var promiseCapability = @newPromiseCapability(@Promise);
promiseCapability.@resolve.@call(undefined, value);
return promiseCapability.@promise;

We may want to add @resolve and @reject slots to the Promise constructor, in which case we would write:
@Promise.@resolve.@call(undefined, value);
Comment 14 Darin Adler 2015-10-31 15:07:41 PDT
Comment on attachment 264396 [details]
Patch

I’m saying review- because I assume you’ll want to address the comments from Youenn and upload a new patch.
Comment 15 Xabier Rodríguez Calvar 2015-11-03 01:51:06 PST
Created attachment 264676 [details]
Patch
Comment 16 Xabier Rodríguez Calvar 2015-11-03 01:58:21 PST
(In reply to comment #12)
> > Source/WebCore/ChangeLog:14
> > +        user, because of the same reason.
> 
> Remove this sentence may make sense since the latest patch does not
> add/remove any @InternalPromise use.
> Renaming the bug title to "[Streams API] Rework promises to use
> @newPromiseCapability" might be nice as well?

Done.

> > Source/WebCore/Modules/streams/ReadableStreamInternals.js:51
> > +        this.@closedPromise = { @promise: Promise.resolve(undefined), @resolve: function() {}, @reject: function() {} };
> 
> We should not need @resolve nor @reject fields.
> Can you try removing them and see whether that works as is?
> If that requires additional changes, let's do that as a follow-up patch.

Those fields can't be removed because the way the spec is (and the code) relies on vending promises that had been already vended before (or created as vended) so in some cases (both in RS and WS) we hit undefined methods because either @reject or @resolve are missing.

> > Source/WebCore/Modules/streams/WritableStream.js:58
> > +    this.@startedPromise = { @promise: Promise.resolve(startResult), @resolve: function() {}, @reject: function() {} };
> 
> Also, in that case, we do not need to use a @promise wrapper.
> I guess this is done for consistency with other fields.
> If code related to startedPromise remains the same or is kept at the same
> place, removing the wrapper makes sense to me.

This is right, startedPromise does not need the wrapper, though my consistency taste says it was better with it.

> Also, the current @XXPromise names are a bit error prone since they no
> longer store promises directly.
> We could use @XXPromiseCapability but this is a bit long.
> Any idea?

I was more into keeping the Promise name even if it was a capability to keep the original spec names. Anyway I renamed them to *PromiseCapability. I don't see any problem in name length.
Comment 17 Xabier Rodríguez Calvar 2015-11-03 02:20:37 PST
(In reply to comment #13)
> One step is to retrieve the Promise constructor object safely.
> Using directly "Promise" is probably not safe.
> "@Promise" seems to refer to a different Promise constructor (not sure of
> its purpose).
> Maybe we can change it so as to get the regular Promise constructor safely?

No, we can't I already tried with @Promise and I manage to modify it from JS, which affects the whole promise behavior. Actually, I suspect that using @Promise can be even worse than using Promise as, I am not sure and I'd love that somebody with deeper JSC knowledge could correct me, but with Promise might refer only to the JS object and @Promise to the WebKit object that will instantiate Promise.

> Based on it, we can implement resolve and reject using code like:
> var promiseCapability = @newPromiseCapability(@Promise);
> promiseCapability.@resolve.@call(undefined, value);
> return promiseCapability.@promise;

If you intention is shielding from a user modifying Promise.prototype.reject and resolve then yes, otherwise I see no point.

I don't know if this implementation could have any impact in the way callbacks are called or the promise resolution chain.

> We may want to add @resolve and @reject slots to the Promise constructor, in
> which case we would write:
> @Promise.@resolve.@call(undefined, value);

We could do something like that, yes, though I would be more included to solve it inside the capability than adding the slots if possible.

About all this, I still don't see the need or shielding all this if the spec says that we have to create promises and promises are subject to be changed by the user through the prototype.
Comment 18 youenn fablet 2015-11-03 03:16:07 PST
r=me

> > > Source/WebCore/Modules/streams/ReadableStreamInternals.js:51
> > > +        this.@closedPromise = { @promise: Promise.resolve(undefined), @resolve: function() {}, @reject: function() {} };
> > 
> > We should not need @resolve nor @reject fields.
> > Can you try removing them and see whether that works as is?
> > If that requires additional changes, let's do that as a follow-up patch.
> 
> Those fields can't be removed because the way the spec is (and the code)
> relies on vending promises that had been already vended before (or created
> as vended) so in some cases (both in RS and WS) we hit undefined methods
> because either @reject or @resolve are missing.

Filed bug 150835 to follow on that.
Comment 19 WebKit Commit Bot 2015-11-03 06:52:57 PST
Comment on attachment 264676 [details]
Patch

Clearing flags on attachment: 264676

Committed r191950: <http://trac.webkit.org/changeset/191950>
Comment 20 WebKit Commit Bot 2015-11-03 06:53:01 PST
All reviewed patches have been landed.  Closing bug.