Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1098663002:
Promise assimilation fix.
DescriptionPromise assimilation fix.
Let x be a fulfilled promise and y be another promise. |x.then(() => y)|
should call |y.then|, but the current implementation calls PromiseChain.
We can see the difference when we set a custom function to |y.then|.
This CL fixes the spec violation, but as a result |then| is no longer
a wrapper of |chain| and in some cases it does not work well with
|accept| or |chain|. That is not a problem for ES6 promise users because
ES6 promise doesn't have them.
LOG=N
BUG=477921
Committed: https://crrev.com/2f57dff3ea0c45e1a61b334fda962460f89d71bc
Cr-Commit-Position: refs/heads/master@{#28926}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : rebase #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : rebase #Patch Set 8 : #Messages
Total messages: 36 (8 generated)
yhirano@chromium.org changed reviewers: + rossberg@chromium.org
https://codereview.chromium.org/1098663002/diff/1/test/webkit/fast/js/Promise... File test/webkit/fast/js/Promise-coerce.js (right): https://codereview.chromium.org/1098663002/diff/1/test/webkit/fast/js/Promise... test/webkit/fast/js/Promise-coerce.js:1: // Copyright 2015 the V8 project authors. All rights reserved. I don't know what license header I should use. Can you tell me?
arv@chromium.org changed reviewers: + arv@chromium.org
https://codereview.chromium.org/1098663002/diff/1/src/promise.js File src/promise.js (right): https://codereview.chromium.org/1098663002/diff/1/src/promise.js#newcode154 src/promise.js:154: IsUnforgedPromise = function IsUnforgedPromise(x) { Does this need to be exported? https://codereview.chromium.org/1098663002/diff/1/src/promise.js#newcode159 src/promise.js:159: return x.then === PromiseThen; This needs a test to ensure that the getter is invoked at the right time and the correct amount of times. https://codereview.chromium.org/1098663002/diff/1/src/promise.js#newcode159 src/promise.js:159: return x.then === PromiseThen; Which section of the ES6 spec defines this? I cannot find anything about this? https://codereview.chromium.org/1098663002/diff/1/test/webkit/fast/js/Promise... File test/webkit/fast/js/Promise-coerce.js (right): https://codereview.chromium.org/1098663002/diff/1/test/webkit/fast/js/Promise... test/webkit/fast/js/Promise-coerce.js:1: // Copyright 2015 the V8 project authors. All rights reserved. test/webkit/ is for tests imported from webkit. New tests should be written in test/mjsunit and in this case test/mjsunit/es6/
Patchset #2 (id:20001) has been deleted
Thanks for the review. I rewrote the implementation and tests. Can you take a look? https://codereview.chromium.org/1098663002/diff/1/src/promise.js File src/promise.js (right): https://codereview.chromium.org/1098663002/diff/1/src/promise.js#newcode154 src/promise.js:154: IsUnforgedPromise = function IsUnforgedPromise(x) { On 2015/04/17 14:32:06, arv wrote: > Does this need to be exported? Deleted. https://codereview.chromium.org/1098663002/diff/1/src/promise.js#newcode159 src/promise.js:159: return x.then === PromiseThen; On 2015/04/17 14:32:06, arv wrote: > Which section of the ES6 spec defines this? I cannot find anything about this? Deleted. https://codereview.chromium.org/1098663002/diff/1/src/promise.js#newcode159 src/promise.js:159: return x.then === PromiseThen; On 2015/04/17 14:32:06, arv wrote: > This needs a test to ensure that the getter is invoked at the right time and the > correct amount of times. Deleted. https://codereview.chromium.org/1098663002/diff/1/test/webkit/fast/js/Promise... File test/webkit/fast/js/Promise-coerce.js (right): https://codereview.chromium.org/1098663002/diff/1/test/webkit/fast/js/Promise... test/webkit/fast/js/Promise-coerce.js:1: // Copyright 2015 the V8 project authors. All rights reserved. On 2015/04/17 14:32:06, arv wrote: > test/webkit/ is for tests imported from webkit. New tests should be written in > test/mjsunit and in this case test/mjsunit/es6/ Done.
ping
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
https://codereview.chromium.org/1098663002/diff/60001/src/promise.js File src/promise.js (right): https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode116 src/promise.js:116: try { deferred.reject(exception); } catch (e) { } Is this not already handled by the enclosing try/catch block? https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode120 src/promise.js:120: deferred.resolve(result); This same branch is repeated a few times, maybe it's worth writing it so that the duplication isn't needed? On the other hand, maybe it's more readable this way. https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode140 src/promise.js:140: for (var i = 0; i < tasks.length; i += 3) { This gets confusing, comments would be helpful
https://codereview.chromium.org/1098663002/diff/60001/src/promise.js File src/promise.js (right): https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode116 src/promise.js:116: try { deferred.reject(exception); } catch (e) { } On 2015/05/11 03:02:16, caitp wrote: > Is this not already handled by the enclosing try/catch block? wait, I get it --- I guess it's not
Is this? https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise-resolve-fun... https://codereview.chromium.org/1098663002/diff/60001/src/promise.js File src/promise.js (right): https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode110 src/promise.js:110: var then = result.then; Isn't this extra Get observable? https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode116 src/promise.js:116: try { deferred.reject(exception); } catch (e) { } On 2015/05/11 at 03:06:05, caitp wrote: > On 2015/05/11 03:02:16, caitp wrote: > > Is this not already handled by the enclosing try/catch block? > > wait, I get it --- I guess it's not I think it is :-) Line 128
https://codereview.chromium.org/1098663002/diff/60001/src/promise.js File src/promise.js (right): https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode116 src/promise.js:116: try { deferred.reject(exception); } catch (e) { } On 2015/05/11 13:53:15, arv wrote: > On 2015/05/11 at 03:06:05, caitp wrote: > > On 2015/05/11 03:02:16, caitp wrote: > > > Is this not already handled by the enclosing try/catch block? > > > > wait, I get it --- I guess it's not > > I think it is :-) > > Line 128 Yeah, but this one is in a closure passed to EnqueueMicrotask, so presumably this doesn't happen right away
https://codereview.chromium.org/1098663002/diff/60001/src/promise.js File src/promise.js (right): https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode110 src/promise.js:110: var then = result.then; On 2015/05/11 13:53:15, arv wrote: > Isn't this extra Get observable? In PromiseReactionJob[1], promiseCapability.[[Resolve]] a.k.a. promise resolve function[2] is called. "then" is accessed in the function, so accessing the property here is correct. 1: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promisereactionjob 2: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise-resolve-fun... https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode120 src/promise.js:120: deferred.resolve(result); On 2015/05/11 03:02:16, caitp wrote: > This same branch is repeated a few times, maybe it's worth writing it so that > the duplication isn't needed? On the other hand, maybe it's more readable this > way. Hmm, I don't have a good idea to unify them (partially because L120 and L125 are in different functions). Do you have one? https://codereview.chromium.org/1098663002/diff/60001/src/promise.js#newcode140 src/promise.js:140: for (var i = 0; i < tasks.length; i += 3) { On 2015/05/11 03:02:16, caitp wrote: > This gets confusing, comments would be helpful Done.
Hm, I'm a bit confused about this change. A couple of questions:
Why is the new code manually running the "then" call in a separate task? Isn't
that asynchronicity encapsulated in the .then method itself already? At least I
don't see this extra step in the spec. AFAICS, it would be sufficient to simply
do
} else if (IsPromise(result)) {
var chain = PromiseChain;
if (thenable) {
chain = result.then;
if (!IS_SPEC_FUNCTION(then)) {
deferred.resolve(result);
return;
}
}
%_CallFunction(result, deferred.resolve, deferred.reject, chain);
}
Why would that not work?
And if you actually do need the duplicated logic, why don't you need to go
through PromiseEnqueue? It seems like you're bypassing all the debugger hooks in
that case. Is that intentional?
On 2015/05/12 13:01:50, rossberg wrote:
> Hm, I'm a bit confused about this change. A couple of questions:
>
> Why is the new code manually running the "then" call in a separate task? Isn't
> that asynchronicity encapsulated in the .then method itself already? At least
I
> don't see this extra step in the spec. AFAICS, it would be sufficient to
simply
> do
>
> } else if (IsPromise(result)) {
> var chain = PromiseChain;
> if (thenable) {
> chain = result.then;
> if (!IS_SPEC_FUNCTION(then)) {
> deferred.resolve(result);
> return;
> }
> }
> %_CallFunction(result, deferred.resolve, deferred.reject, chain);
> }
>
> Why would that not work?
My understanding is that calling |then| should be in a separate task. Please see
the 12th step in
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise-resolve-fun...
and correct me if I'm wrong.
>
> And if you actually do need the duplicated logic, why don't you need to go
> through PromiseEnqueue? It seems like you're bypassing all the debugger hooks
in
> that case. Is that intentional?
Ah, you mean calling
PromiseEnqueue(result, [then, deferred, true], +1)
? That would be a good idea. If my understanding for the first question is
correct, I would do that.
On 2015/05/15 01:34:32, yhirano wrote:
> On 2015/05/12 13:01:50, rossberg wrote:
> > Hm, I'm a bit confused about this change. A couple of questions:
> >
> > Why is the new code manually running the "then" call in a separate task?
Isn't
> > that asynchronicity encapsulated in the .then method itself already? At
least
> I
> > don't see this extra step in the spec. AFAICS, it would be sufficient to
> simply
> > do
> >
> > } else if (IsPromise(result)) {
> > var chain = PromiseChain;
> > if (thenable) {
> > chain = result.then;
> > if (!IS_SPEC_FUNCTION(then)) {
> > deferred.resolve(result);
> > return;
> > }
> > }
> > %_CallFunction(result, deferred.resolve, deferred.reject, chain);
> > }
> >
> > Why would that not work?
> My understanding is that calling |then| should be in a separate task. Please
see
> the 12th step in
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise-resolve-fun...
> and correct me if I'm wrong.
Hm, you are right. This is really odd, though. Seems like an unnecessary
complication and overhead to take an extra turn there.
> > And if you actually do need the duplicated logic, why don't you need to go
> > through PromiseEnqueue? It seems like you're bypassing all the debugger
hooks
> in
> > that case. Is that intentional?
> Ah, you mean calling
>
> PromiseEnqueue(result, [then, deferred, true], +1)
>
> ? That would be a good idea. If my understanding for the first question is
> correct, I would do that.
> On 2015/05/15 01:34:32, yhirano wrote:
> > On 2015/05/12 13:01:50, rossberg wrote:
> > > Hm, I'm a bit confused about this change. A couple of questions:
> > >
> > > Why is the new code manually running the "then" call in a separate task?
> Isn't
> > > that asynchronicity encapsulated in the .then method itself already? At
> least
> > I
> > > don't see this extra step in the spec. AFAICS, it would be sufficient to
> > simply
> > > do
> > >
> > > } else if (IsPromise(result)) {
> > > var chain = PromiseChain;
> > > if (thenable) {
> > > chain = result.then;
> > > if (!IS_SPEC_FUNCTION(then)) {
> > > deferred.resolve(result);
> > > return;
> > > }
> > > }
> > > %_CallFunction(result, deferred.resolve, deferred.reject, chain);
> > > }
> > >
> > > Why would that not work?
> > My understanding is that calling |then| should be in a separate task. Please
> see
> > the 12th step in
> >
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise-resolve-fun...
> > and correct me if I'm wrong.
>
> Hm, you are right. This is really odd, though. Seems like an unnecessary
> complication and overhead to take an extra turn there.
>
Yeah, I agree that it's odd.
> > > And if you actually do need the duplicated logic, why don't you need to go
> > > through PromiseEnqueue? It seems like you're bypassing all the debugger
> hooks
> > in
> > > that case. Is that intentional?
> > Ah, you mean calling
> >
> > PromiseEnqueue(result, [then, deferred, true], +1)
> >
> > ? That would be a good idea. If my understanding for the first question is
> > correct, I would do that.
I was wrong on this point: I cannot simply reuse PromiseEnqueue because its
requirement is different from what the code block wants to do. Should I
duplicate the all debugger hooks in PromiseEnqueue?
On 2015/05/20 11:59:41, yhirano wrote: > > On 2015/05/15 01:34:32, yhirano wrote: > > > Ah, you mean calling > > > > > > PromiseEnqueue(result, [then, deferred, true], +1) > > > > > > ? That would be a good idea. If my understanding for the first question is > > > correct, I would do that. > > I was wrong on this point: I cannot simply reuse PromiseEnqueue because its > requirement is different from what the code block wants to do. Should I > duplicate the all debugger hooks in PromiseEnqueue? Yes, you need to, otherwise devtools will not work correctly. Maybe this can be refactored a little bit to share as much code as possible?
On 2015/05/20 12:48:37, rossberg wrote: > On 2015/05/20 11:59:41, yhirano wrote: > > I was wrong on this point: I cannot simply reuse PromiseEnqueue because its > > requirement is different from what the code block wants to do. Should I > > duplicate the all debugger hooks in PromiseEnqueue? > > Yes, you need to, otherwise devtools will not work correctly. Maybe this can be > refactored a little bit to share as much code as possible? Thanks. I decoupled the EnqueueMicrotask call and added debug instructions. I think not sharing code is straightforward in this case because the code is not much and many names are slightly different. What do you think?
LGTM [Sorry for the delay, I was on vacation.]
Can you land this CL? I'm not a V8 committer.
The CQ bit was checked by rossberg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098663002/120001
On 2015/06/10 07:33:52, yhirano wrote: > Can you land this CL? I'm not a V8 committer. Ah, we now have a working commit queue, so this problem should be gone. You just check the button (I just did).
On 2015/06/10 08:03:15, rossberg wrote: > On 2015/06/10 07:33:52, yhirano wrote: > > Can you land this CL? I'm not a V8 committer. > > Ah, we now have a working commit queue, so this problem should be gone. You just > check the button (I just did). Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1098663002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1098663002/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2f57dff3ea0c45e1a61b334fda962460f89d71bc Cr-Commit-Position: refs/heads/master@{#28926}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:160001) has been created in https://codereview.chromium.org/1176163004/ by yangguo@chromium.org. The reason for reverting is: Test failures: https://chromegw.corp.google.com/i/client.v8/builders/V8%20Linux64/builds/3829.
Message was sent while issue was closed.
On 2015/06/11 08:00:54, Yang wrote: > A revert of this CL (patchset #8 id:160001) has been created in > https://codereview.chromium.org/1176163004/ by mailto:yangguo@chromium.org. > > The reason for reverting is: Test failures: > https://chromegw.corp.google.com/i/client.v8/builders/V8%20Linux64/builds/3829. I relanded this CL. It's due to an issue with the external snapshot. The snapshot blob and the natives blob became out of sync.
Reopen as Revert of Revert of Revert is landed. https://codereview.chromium.org/1181603003/ |
