Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1578893002:
Partial rollback of Promise error checking (Closed)
|
Created:
4 years, 11 months ago by Dan Ehrenberg Modified:
4 years, 11 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionPartial rollback of Promise error checking
As V8 becomes more and more spec-compliant, Promise polyfill libraries
like core.js expect fully correct. However, our Promises do not yet
support Symbol.species. Therefore, a case like
```
var test = new Promise(function(){});
test.constructor = function(){};
Promise.resolve(test)
```
would lead to an unhandled Promise rejection, whereas it should not
because test.constructor[Symbol.species] is undefined, so test.then
should end up constructing %Promise% as a fallback, rather than
calling test.constructor as if it were a constructor, which leads
this error checking code to throw.
For now, this patch removes the error checking code (which was not
present until recently). In an interactive test using core.js, the
error message on the console goes away with this patch. When @@species
support is in place, this patch can be reverted. A regression test
is added which checks for the same thing.
Partially reverted patch was originally out for review at
https://codereview.chromium.org/1531073004
BUG=v8:4633
LOG=Y
R=adamk,caitp88@gmail.com
Committed: https://crrev.com/ee9d7acafc2996d1dad6e97bd04d852fd67bf974
Cr-Commit-Position: refs/heads/master@{#33217}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix test expectations #Patch Set 3 : Remove unused variable from test #Patch Set 4 : Add failing ignition test #
Messages
Total messages: 29 (12 generated)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578893002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578893002/1
Description was changed from
==========
Partial rollback of Promise error checking
As V8 becomes more and more spec-compliant, Promise polyfill libraries
like core.js expect fully correct. However, our Promises do not yet
support Symbol.species. Therefore, a case like
```
var test = new Promise(function(){});
test.constructor = function(){};
Promise.resolve(test)
```
would lead to an unhandled Promise rejection, whereas it should not
because test.constructor[Symbol.species] is undefined, so test.then
should end up constructing %Promise% as a fallback, rather than
calling test.constructor as if it were a constructor, which leads
this error checking code to throw.
FOr now, this patch removes the error checking code (which was not
present until recently). In an interactive test using core.js, the
error message on the console goes away with this patch. When @@species
support is in place, this patch can be reverted.
Partially reverted patch was originally out for review at
https://codereview.chromium.org/1531073004
BUG=v8:4633
LOG=Y
R=adamk,caitp88@gmail.com
==========
to
==========
Partial rollback of Promise error checking
As V8 becomes more and more spec-compliant, Promise polyfill libraries
like core.js expect fully correct. However, our Promises do not yet
support Symbol.species. Therefore, a case like
```
var test = new Promise(function(){});
test.constructor = function(){};
Promise.resolve(test)
```
would lead to an unhandled Promise rejection, whereas it should not
because test.constructor[Symbol.species] is undefined, so test.then
should end up constructing %Promise% as a fallback, rather than
calling test.constructor as if it were a constructor, which leads
this error checking code to throw.
For now, this patch removes the error checking code (which was not
present until recently). In an interactive test using core.js, the
error message on the console goes away with this patch. When @@species
support is in place, this patch can be reverted. A regression test
is added which checks for the same thing.
Partially reverted patch was originally out for review at
https://codereview.chromium.org/1531073004
BUG=v8:4633
LOG=Y
R=adamk,caitp88@gmail.com
==========
adamk@chromium.org changed reviewers: + caitpotter88@gmail.com - caitp88@gmail.com
lgtm % unused variable in test https://codereview.chromium.org/1578893002/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-crbug-575314.js (right): https://codereview.chromium.org/1578893002/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-crbug-575314.js:15: var exception; This var looks unused?
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578893002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578893002/20001
https://codereview.chromium.org/1578893002/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-crbug-575314.js (right): https://codereview.chromium.org/1578893002/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-crbug-575314.js:15: var exception; On 2016/01/11 at 20:17:17, adamk wrote: > This var looks unused? Fixed.
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/1578893002/#ps40001 (title: "Remove unused variable from test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578893002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578893002/40001
I'm not seeing how @@species impacts those tests at all, can you elaborate on that?
On 2016/01/11 20:21:31, caitp wrote: > I'm not seeing how @@species impacts those tests at all, can you elaborate on > that? Or is this just a "We should keep doing the wrong thing until we're doing the right thing everywhere" kind of thing
On 2016/01/11 20:21:31, caitp wrote: > I'm not seeing how @@species impacts those tests at all, can you elaborate on > that? Or is this just a "We should keep doing the wrong thing until we're doing the right thing everywhere" kind of thing
On 2016/01/11 at 20:23:34, caitpotter88 wrote: > On 2016/01/11 20:21:31, caitp wrote: > > I'm not seeing how @@species impacts those tests at all, can you elaborate on > > that? > > Or is this just a "We should keep doing the wrong thing until we're doing the right thing everywhere" kind of thing test.constructor doesn't have a @@species property, so we'll fall back to %Promise%, which won't cause those checks to fail. This whole path is invoked because Promise.resolve calls the [[Resolve]] method on the NewPromiseCapability, which calls .then, which constructs a new promise with SpeciesConstructor. You could see this as an example of "do the wrong thing until we do the right thing anywhere". I'm not so happy about this, but we have a concrete web compatibility issue to worry about. I'd rather not have an uncaught promise rejection error message on all core.js users who can't manage to upgrade to the latest version over the next six weeks. Ultimately, please blame me for this, for not implementing @@species faster.
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...)
On 2016/01/11 20:29:53, Dan Ehrenberg wrote: > On 2016/01/11 at 20:23:34, caitpotter88 wrote: > > On 2016/01/11 20:21:31, caitp wrote: > > > I'm not seeing how @@species impacts those tests at all, can you elaborate > on > > > that? > > > > Or is this just a "We should keep doing the wrong thing until we're doing the > right thing everywhere" kind of thing > > test.constructor doesn't have a @@species property, so we'll fall back to > %Promise%, which won't cause those checks to fail. This whole path is invoked > because Promise.resolve calls the [[Resolve]] method on the > NewPromiseCapability, which calls .then, which constructs a new promise with > SpeciesConstructor. > This isn't actually, true, per spec. @@species has no impact whatsoever on NewPromiseCapability(). Promise.prototype.then() is the only Promise method which actually uses SpeciesConstructor(), and that use is not even represented by this test. > You could see this as an example of "do the wrong thing until we do the right > thing anywhere". I'm not so happy about this, but we have a concrete web > compatibility issue to worry about. I'd rather not have an uncaught promise > rejection error message on all core.js users who can't manage to upgrade to the > latest version over the next six weeks. Ultimately, please blame me for this, > for not implementing @@species faster.
On 2016/01/11 21:00:06, caitp wrote: > On 2016/01/11 20:29:53, Dan Ehrenberg wrote: > > On 2016/01/11 at 20:23:34, caitpotter88 wrote: > > > On 2016/01/11 20:21:31, caitp wrote: > > > > I'm not seeing how @@species impacts those tests at all, can you elaborate > > on > > > > that? > > > > > > Or is this just a "We should keep doing the wrong thing until we're doing > the > > right thing everywhere" kind of thing > > > > test.constructor doesn't have a @@species property, so we'll fall back to > > %Promise%, which won't cause those checks to fail. This whole path is invoked > > because Promise.resolve calls the [[Resolve]] method on the > > NewPromiseCapability, which calls .then, which constructs a new promise with > > SpeciesConstructor. > > > > This isn't actually, true, per spec. @@species has no impact whatsoever on > NewPromiseCapability(). Promise.prototype.then() is the only Promise method > which actually uses SpeciesConstructor(), and that use is not even represented > by this test. > > > You could see this as an example of "do the wrong thing until we do the right > > thing anywhere". I'm not so happy about this, but we have a concrete web > > compatibility issue to worry about. I'd rather not have an uncaught promise > > rejection error message on all core.js users who can't manage to upgrade to > the > > latest version over the next six weeks. Ultimately, please blame me for this, > > for not implementing @@species faster. Alright hang on, I've misread the test, you're right that @@species would have an impact there.
On 2016/01/11 at 21:01:45, caitpotter88 wrote: > On 2016/01/11 21:00:06, caitp wrote: > > On 2016/01/11 20:29:53, Dan Ehrenberg wrote: > > > On 2016/01/11 at 20:23:34, caitpotter88 wrote: > > > > On 2016/01/11 20:21:31, caitp wrote: > > > > > I'm not seeing how @@species impacts those tests at all, can you elaborate > > > on > > > > > that? > > > > > > > > Or is this just a "We should keep doing the wrong thing until we're doing > > the > > > right thing everywhere" kind of thing > > > > > > test.constructor doesn't have a @@species property, so we'll fall back to > > > %Promise%, which won't cause those checks to fail. This whole path is invoked > > > because Promise.resolve calls the [[Resolve]] method on the > > > NewPromiseCapability, which calls .then, which constructs a new promise with > > > SpeciesConstructor. > > > > > > > This isn't actually, true, per spec. @@species has no impact whatsoever on > > NewPromiseCapability(). Promise.prototype.then() is the only Promise method > > which actually uses SpeciesConstructor(), and that use is not even represented > > by this test. > > > > > You could see this as an example of "do the wrong thing until we do the right > > > thing anywhere". I'm not so happy about this, but we have a concrete web > > > compatibility issue to worry about. I'd rather not have an uncaught promise > > > rejection error message on all core.js users who can't manage to upgrade to > > the > > > latest version over the next six weeks. Ultimately, please blame me for this, > > > for not implementing @@species faster. > > Alright hang on, I've misread the test, you're right that @@species would have an impact there. then gets called by [[Resolve]]
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578893002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578893002/60001
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/1578893002/#ps60001 (title: "Add failing ignition test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1578893002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1578893002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
Partial rollback of Promise error checking
As V8 becomes more and more spec-compliant, Promise polyfill libraries
like core.js expect fully correct. However, our Promises do not yet
support Symbol.species. Therefore, a case like
```
var test = new Promise(function(){});
test.constructor = function(){};
Promise.resolve(test)
```
would lead to an unhandled Promise rejection, whereas it should not
because test.constructor[Symbol.species] is undefined, so test.then
should end up constructing %Promise% as a fallback, rather than
calling test.constructor as if it were a constructor, which leads
this error checking code to throw.
For now, this patch removes the error checking code (which was not
present until recently). In an interactive test using core.js, the
error message on the console goes away with this patch. When @@species
support is in place, this patch can be reverted. A regression test
is added which checks for the same thing.
Partially reverted patch was originally out for review at
https://codereview.chromium.org/1531073004
BUG=v8:4633
LOG=Y
R=adamk,caitp88@gmail.com
==========
to
==========
Partial rollback of Promise error checking
As V8 becomes more and more spec-compliant, Promise polyfill libraries
like core.js expect fully correct. However, our Promises do not yet
support Symbol.species. Therefore, a case like
```
var test = new Promise(function(){});
test.constructor = function(){};
Promise.resolve(test)
```
would lead to an unhandled Promise rejection, whereas it should not
because test.constructor[Symbol.species] is undefined, so test.then
should end up constructing %Promise% as a fallback, rather than
calling test.constructor as if it were a constructor, which leads
this error checking code to throw.
For now, this patch removes the error checking code (which was not
present until recently). In an interactive test using core.js, the
error message on the console goes away with this patch. When @@species
support is in place, this patch can be reverted. A regression test
is added which checks for the same thing.
Partially reverted patch was originally out for review at
https://codereview.chromium.org/1531073004
BUG=v8:4633
LOG=Y
R=adamk,caitp88@gmail.com
Committed: https://crrev.com/ee9d7acafc2996d1dad6e97bd04d852fd67bf974
Cr-Commit-Position: refs/heads/master@{#33217}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ee9d7acafc2996d1dad6e97bd04d852fd67bf974 Cr-Commit-Position: refs/heads/master@{#33217} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
