Bug 150627 – [Streams API] Rework promises to use @newPromiseCapability

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

[Streams API] Rework promises to use @newPromiseCapabilites and as much InternalPromise as possible

[Xabier Rodríguez Calvar]

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.

[youenn fablet]

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?

[Xabier Rodríguez Calvar]

(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.

[youenn fablet]

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?

[Yusuke Suzuki]

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?

[Xabier Rodríguez Calvar]

(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!

[Yusuke Suzuki]

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.

[Xabier Rodríguez Calvar]

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.

[youenn fablet]

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.

[Xabier Rodríguez Calvar]

Created attachment 264396 [details]
Patch

[Xabier Rodríguez Calvar]

(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.

[youenn fablet]

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.

[youenn fablet]

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);

[Darin Adler]

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.

[Xabier Rodríguez Calvar]

Created attachment 264676 [details]
Patch

[Xabier Rodríguez Calvar]

(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.

[Xabier Rodríguez Calvar]

(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.

[youenn fablet]

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.

[WebKit Commit Bot]

Comment on attachment 264676 [details]
Patch

Clearing flags on attachment: 264676

Committed r191950: <http://trac.webkit.org/changeset/191950>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.