Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2212963002:
[elements] update Dictionary in IncludesValue if own elements change (Closed)
Description[elements] update Dictionary in IncludesValue if own elements change
Ensure that receiver->elements() == *dictionary after calling an accessor, in
addition to checking the prototype.
BUG=chromium:634273
, chromium: 634357, v8:5162
R=cbruni@chromium.org, mstarzinger@chromium.org
Committed: https://crrev.com/9977a2caf30329e60bed69c459e19d85f162abfe
Cr-Commit-Position: refs/heads/master@{#38347}
Patch Set 1 #
Total comments: 7
Patch Set 2 : address nits #Patch Set 3 : rebase #Patch Set 4 : Just reset the dictionary, don't bailout #Patch Set 5 : fix the test #Patch Set 6 : Add test for other related failure #
Messages
Total messages: 29 (21 generated)
PTAL
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [elements] bailout from DICTIONARY_ELEMENTS IncludesValue if elements change Ensure that receiver->elemnets() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ========== to ========== [elements] bailout from DICTIONARY_ELEMENTS IncludesValue if elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/2212963002/diff/1/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2212963002/diff/1/src/elements.cc#newcode1544 src/elements.cc:1544: if (*dictionary != receiver->elements() || as long as we stay in dictionary mode (which is always the case as we do not migrate dict-elements to fast elements) you could just refresh the dictionary (probably won't matter though in this corner case). https://codereview.chromium.org/2212963002/diff/1/test/mjsunit/es7/regress/re... File test/mjsunit/es7/regress/regress-634273.js (right): https://codereview.chromium.org/2212963002/diff/1/test/mjsunit/es7/regress/re... test/mjsunit/es7/regress/regress-634273.js:9: var v10 = eval(); nit: can you clean up the test case a bit? Otherwise it looks like all these things are a requirement :) (v7-v10) can just be replace with undefined https://codereview.chromium.org/2212963002/diff/1/test/mjsunit/es7/regress/re... test/mjsunit/es7/regress/regress-634273.js:11: v18 = v13.copyWithin(); drop this. https://codereview.chromium.org/2212963002/diff/1/test/mjsunit/es7/regress/re... test/mjsunit/es7/regress/regress-634273.js:13: Object.defineProperty(v18, 0, { v18 => v13 https://codereview.chromium.org/2212963002/diff/1/test/mjsunit/es7/regress/re... test/mjsunit/es7/regress/regress-634273.js:19: v48 = v13.includes(v35); v35 is just a value that isn't in the array
https://codereview.chromium.org/2212963002/diff/1/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2212963002/diff/1/src/elements.cc#newcode1544 src/elements.cc:1544: if (*dictionary != receiver->elements() || On 2016/08/04 15:51:57, Camillo Bruni wrote: > as long as we stay in dictionary mode (which is always the case as we do not > migrate dict-elements to fast elements) you could just refresh the dictionary > (probably won't matter though in this corner case). You're saying the elements kind can't change, so just update the handle instead of bailing out, right? will do
Description was changed from ========== [elements] bailout from DICTIONARY_ELEMENTS IncludesValue if elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ========== to ========== [elements] update Dictionary in IncludesValue if own elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ==========
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by caitp@igalia.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated to fix a related failure https://codereview.chromium.org/2212963002/diff/1/src/elements.cc File src/elements.cc (right): https://codereview.chromium.org/2212963002/diff/1/src/elements.cc#newcode1544 src/elements.cc:1544: if (*dictionary != receiver->elements() || On 2016/08/04 16:13:06, caitp wrote: > On 2016/08/04 15:51:57, Camillo Bruni wrote: > > as long as we stay in dictionary mode (which is always the case as we do not > > migrate dict-elements to fast elements) you could just refresh the dictionary > > (probably won't matter though in this corner case). > > You're saying the elements kind can't change, so just update the handle instead > of bailing out, right? will do This seems to be untrue, JSObject::ResetElements() (called if `length` is 0 in ElementsAccessor::SetLength()) can reset the elements kind and fixed array to something empty. There might be other cases that could break, too.
Description was changed from ========== [elements] update Dictionary in IncludesValue if own elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ========== to ========== [elements] update Dictionary in IncludesValue if own elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, chromium: 634357, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Latest changes TBR=cbruni, but I think you'll probably be okay with them.
The CQ bit was checked by caitp@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org Link to the patchset: https://codereview.chromium.org/2212963002/#ps100001 (title: "Add test for other related failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [elements] update Dictionary in IncludesValue if own elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, chromium: 634357, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ========== to ========== [elements] update Dictionary in IncludesValue if own elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, chromium: 634357, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [elements] update Dictionary in IncludesValue if own elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, chromium: 634357, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org ========== to ========== [elements] update Dictionary in IncludesValue if own elements change Ensure that receiver->elements() == *dictionary after calling an accessor, in addition to checking the prototype. BUG=chromium:634273, chromium: 634357, v8:5162 R=cbruni@chromium.org, mstarzinger@chromium.org Committed: https://crrev.com/9977a2caf30329e60bed69c459e19d85f162abfe Cr-Commit-Position: refs/heads/master@{#38347} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9977a2caf30329e60bed69c459e19d85f162abfe Cr-Commit-Position: refs/heads/master@{#38347} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
