Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 9 months ago by caitp (gmail) Modified:
5 years, 8 months ago Reviewers:
arv (Not doing code reviews) CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[es6] don't throw if argument is non-object (O.freeze, O.seal, O.preventExtensions)
BUG=v8:3965, v8:3966
R=arv@chromium.org
LOG=N
Committed: https://crrev.com/b09c048f693d280052ac63c7d6b3baf27b3bf271
Cr-Commit-Position: refs/heads/master@{#27985}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fix incorrect tests #Patch Set 3 : Rebase #Patch Set 4 : Fix test #Patch Set 5 : Add test262 exceptions #Patch Set 6 : Do a better job on the test262 exceptions #Patch Set 7 : remove phantom edit #
Created: 5 years, 8 months ago
Messages
Total messages: 36 (15 generated)
PTAL --- this could use a try run so that we know if test262/mozilla statuses need to change
arv@chromium.org changed reviewers: + arv@chromium.org
https://codereview.chromium.org/1011823003/diff/1/test/mjsunit/object-freeze.js File test/mjsunit/object-freeze.js (right): https://codereview.chromium.org/1011823003/diff/1/test/mjsunit/object-freeze.... test/mjsunit/object-freeze.js:340: (function propertiesOfFrozenObjectNotFrozen() { How are these tests related to this CL? https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... File test/webkit/preventExtensions.js (right): https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... test/webkit/preventExtensions.js:95: shouldBe('var o = {}; Object.preventExtensions(o); o.__proto__ = { newProp: "Should not see this" }; o.newProp;'); This looks strange. shouldBe takes two strings that are evaluated and compared. https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... test/webkit/preventExtensions.js:96: shouldBe('"use strict"; var o = {}; Object.preventExtensions(o); o.__proto__ = { newProp: "Should not see this" }; o.newProp;'); This should still throw, shouldn't it?
https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... File test/webkit/preventExtensions.js (right): https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... test/webkit/preventExtensions.js:96: shouldBe('"use strict"; var o = {}; Object.preventExtensions(o); o.__proto__ = { newProp: "Should not see this" }; o.newProp;'); On 2015/03/19 19:38:08, arv wrote: > This should still throw, shouldn't it? Yeah. I passed make check, but I guess the WebKit tests aren't run that way. I'll you're both of these tonight
https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... File test/webkit/preventExtensions.js (right): https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... test/webkit/preventExtensions.js:95: shouldBe('var o = {}; Object.preventExtensions(o); o.__proto__ = { newProp: "Should not see this" }; o.newProp;'); On 2015/03/19 19:38:08, arv wrote: > This looks strange. shouldBe takes two strings that are evaluated and compared. Looks like in this case, it should throw https://people.mozilla.org/~jorendorff/es6-draft.html#sec-set-object.prototyp... --- but for other properties it shouldn't.
Patchset #2 (id:20001) has been deleted
On 2015/03/19 19:42:10, caitp wrote: > https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... > File test/webkit/preventExtensions.js (right): > > https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... > test/webkit/preventExtensions.js:96: shouldBe('"use strict"; var o = {}; > Object.preventExtensions(o); o.__proto__ = { newProp: "Should not see this" }; > o.newProp;'); > On 2015/03/19 19:38:08, arv wrote: > > This should still throw, shouldn't it? > > Yeah. I passed make check, but I guess the WebKit tests aren't run that way. > I'll you're both of these tonight tools/run-tests.py --mode release --arch x64 webkit
On 2015/03/19 20:30:15, arv wrote: > On 2015/03/19 19:42:10, caitp wrote: > > > https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... > > File test/webkit/preventExtensions.js (right): > > > > > https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... > > test/webkit/preventExtensions.js:96: shouldBe('"use strict"; var o = {}; > > Object.preventExtensions(o); o.__proto__ = { newProp: "Should not see this" }; > > o.newProp;'); > > On 2015/03/19 19:38:08, arv wrote: > > > This should still throw, shouldn't it? > > > > Yeah. I passed make check, but I guess the WebKit tests aren't run that way. > > I'll you're both of these tonight > > tools/run-tests.py --mode release --arch x64 webkit Yeah, it looks safe to leave the WebKit tests as they were, they are correct. All fixed up, passing locally. Just maybe test262 statuses need an update
On 2015/03/19 20:33:21, caitp wrote: > On 2015/03/19 20:30:15, arv wrote: > > On 2015/03/19 19:42:10, caitp wrote: > > > > > > https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... > > > File test/webkit/preventExtensions.js (right): > > > > > > > > > https://codereview.chromium.org/1011823003/diff/1/test/webkit/preventExtensio... > > > test/webkit/preventExtensions.js:96: shouldBe('"use strict"; var o = {}; > > > Object.preventExtensions(o); o.__proto__ = { newProp: "Should not see this" > }; > > > o.newProp;'); > > > On 2015/03/19 19:38:08, arv wrote: > > > > This should still throw, shouldn't it? > > > > > > Yeah. I passed make check, but I guess the WebKit tests aren't run that way. > > > I'll you're both of these tonight > > > > tools/run-tests.py --mode release --arch x64 webkit > > Yeah, it looks safe to leave the WebKit tests as they were, they are correct. > All fixed up, passing locally. Just maybe test262 statuses need an update ping arv@ --- kind of forgot about this one, but it seems trivial
LGTM Maybe add chrome/blink to the try bots?
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011823003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...) v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/5022)
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1011823003/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011823003/60001
The CQ bit was unchecked by caitpotter88@gmail.com
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1011823003/#ps80001 (title: "Fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011823003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/5037)
On 2015/04/21 19:44:41, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1011823003/80001 groannnnn :| test262 :<
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1011823003/#ps100001 (title: "Add test262 exceptions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011823003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/5040)
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1011823003/#ps140001 (title: "remove phantom edit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011823003/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b09c048f693d280052ac63c7d6b3baf27b3bf271 Cr-Commit-Position: refs/heads/master@{#27985}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:140001) has been created in https://codereview.chromium.org/1103473003/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] breaks mac gc stress: http://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds/....
Message was sent while issue was closed.
On 2015/04/22 07:57:36, Michael Achenbach wrote: > A revert of this CL (patchset #7 id:140001) has been created in > https://codereview.chromium.org/1103473003/ by mailto:machenbach@chromium.org. > > The reason for reverting is: [Sheriff] breaks mac gc stress: > http://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds/.... I'm unable to reproduce these test failures on any of my mac devices, following the recipe for the gc stress bot. I'm a bit skeptical that these are related to breakage |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
