Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2664173002:
Throw when a holey property is set in Array.sort (Closed)
|
Created:
3 years, 10 months ago by Choongwoo Han Modified:
3 years, 10 months ago Reviewers:
adamk CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionThrow when a holey property is set in Array.sort
Do not allow that holey properties are defined in Array sort.
Throw a type error if the array is not extensible and there are holey
properties in the middle of the array.
BUG=v8:4888
Review-Url: https://codereview.chromium.org/2664173002
Cr-Commit-Position: refs/heads/master@{#43126}
Committed: https://chromium.googlesource.com/v8/v8/+/48dff523f7348af657d610f789d326d85f84ecec
Patch Set 1 #Patch Set 2 : Throw when a holey property is set in Array.sort #Patch Set 3 : Use hasOwnProperties for checking holes #Patch Set 4 : Use preventExtensions for test #
Total comments: 2
Patch Set 5 : Patch PrepareSlowElementsForSort directly #
Total comments: 7
Patch Set 6 : Handling undefs #Patch Set 7 : Not allow adding propertes in non-extensible array #
Messages
Total messages: 28 (11 generated)
Description was changed from ========== Throw when a holey property is set in Array.sort Do not allow that holey properties are defined in Array sort. Throw a type error if the array is not extensible and there are holey properties in the middle of the array. BUG=v8:4888 ========== to ========== Throw when a holey property is set in Array.sort Do not allow that holey properties are defined in Array sort. Throw a type error if the array is not extensible and there are holey properties in the middle of the array. BUG=v8:4888 ==========
cwhan.tunz@gmail.com changed reviewers: + adamk@chromium.org
We need to prevent modification of properties in array sort for non-extensible arrays. Thus, I checked if the array have holes or not in the middle of the array before starting sort.
We need to prevent modification of properties in array sort for non-extensible arrays. Thus, I checked if the array have holes or not in the middle of the array before starting sort.
The CQ bit was checked by cwhan.tunz@gmail.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Thanks for the patch, but this approach looks like it's not attacking the real source of the problem, which is that the runtime function %RemoveArrayHoles (implemented in JSObject::PrepareElementsForSort) doesn't take into account extensibility of the object. What happens if you make that return -1 for non-extensible objects just as it does for other failure cases? It looks to me like SafeRemoveArrayHoles might actually just throw the TypeError for us. https://codereview.chromium.org/2664173002/diff/60001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/2664173002/diff/60001/test/mjsunit/array-sort... test/mjsunit/array-sort.js:502: assertEquals(arr, [1,2,undefined]); Shouldn't this fail now?
On 2017/02/01 16:39:13, adamk wrote: > Thanks for the patch, but this approach looks like it's not attacking the real > source of the problem, which is that the runtime function %RemoveArrayHoles > (implemented in JSObject::PrepareElementsForSort) doesn't take into account > extensibility of the object. > Yes, it is. But, when SafeRemoveArrayHoles is used to remove array holes, it will give a wrong result instead of throwing a TypeError since [[Set]] in non-extensible array does not throw a TypeError. For example, var arr = [1,,2]; Object.preventExtensions(arr); arr[1] = 123; arr[1] == undefined . So, I thought it is okey because RemoveArrayHoles is only used for this case. Do you think we need to patch the real source in RemoveArrayHoles, and we need another check for SafeRemoveArrayHoles? > What happens if you make that return -1 for non-extensible objects just as it > does for other failure cases? It looks to me like SafeRemoveArrayHoles might > actually just throw the TypeError for us. > > https://codereview.chromium.org/2664173002/diff/60001/test/mjsunit/array-sort.js > File test/mjsunit/array-sort.js (right): > > https://codereview.chromium.org/2664173002/diff/60001/test/mjsunit/array-sort... > test/mjsunit/array-sort.js:502: assertEquals(arr, [1,2,undefined]); > Shouldn't this fail now?
https://codereview.chromium.org/2664173002/diff/60001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/2664173002/diff/60001/test/mjsunit/array-sort... test/mjsunit/array-sort.js:502: assertEquals(arr, [1,2,undefined]); On 2017/02/01 16:39:13, adamk wrote: > Shouldn't this fail now? Yes. In this case, this arr does not have any hole since undefined also has a key ([1, undefined, 2].hasOwnProperty(1) == true).
Hi, I pushed a new patch. As you said, returning -1 throws TypeError. I still don't know exactly why. maybe I need to study more. I only fixed PrepareSlowElementsForSort since making array non-extensible also make the array to slow elements. bailout when a new key is added for non-extensible arrays. PTAL
Apologies for the review delay. The reason for the TypeError is that the Array builtins are in strict mode, so any object mutations that fail throw an error rather than failing silently. https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc#newcode1... src/objects.cc:16802: bool is_extensible = JSObject::IsExtensible(object); Please add a DCHECK to JSObject::PrepareElementsForSort, like DCHECK(JSObject::IsExtensible(object)) This will make sure that even if we in the future can make a non-extensible object have fast elements, the code will keep working. https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc#newcode1... src/objects.cc:16817: Handle<Object> value(dict->ValueAt(i), isolate); Rather than changing the logic below, couldn't you simply bail out here if you've found a hole? if (value->IsTheHole(isolate)) { return bailout; } https://codereview.chromium.org/2664173002/diff/80001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/2664173002/diff/80001/test/mjsunit/array-sort... test/mjsunit/array-sort.js:494: assertTrue(exception); You can use assertThrows(() => arr.sort(), TypeError) to do this try/catch logic for you.
Thank you for the feedback! https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc#newcode1... src/objects.cc:16802: bool is_extensible = JSObject::IsExtensible(object); On 2017/02/07 00:23:46, adamk wrote: > Please add a DCHECK to JSObject::PrepareElementsForSort, like > > DCHECK(JSObject::IsExtensible(object)) > > This will make sure that even if we in the future can make a non-extensible > object have fast elements, the code will keep working. Done. https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc#newcode1... src/objects.cc:16817: Handle<Object> value(dict->ValueAt(i), isolate); On 2017/02/07 00:23:46, adamk wrote: > Rather than changing the logic below, couldn't you simply bail out here if > you've found a hole? > > if (value->IsTheHole(isolate)) { > return bailout; > } Since it is a dictionary iteration, value cannot be a hole. We should check if the new key (pos) is the hole or not. So, I can bail out in two positions like line 16828 and 16839 did in the original code if we don't want to refactor this code. https://codereview.chromium.org/2664173002/diff/80001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/2664173002/diff/80001/test/mjsunit/array-sort... test/mjsunit/array-sort.js:494: assertTrue(exception); On 2017/02/07 00:23:46, adamk wrote: > You can use > > assertThrows(() => arr.sort(), TypeError) > > to do this try/catch logic for you. Done.
I added one more test case for undef cases. bail out in three different locations checking new position.
https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc#newcode1... src/objects.cc:16817: Handle<Object> value(dict->ValueAt(i), isolate); On 2017/02/07 09:45:49, tunz wrote: > On 2017/02/07 00:23:46, adamk wrote: > > Rather than changing the logic below, couldn't you simply bail out here if > > you've found a hole? > > > > if (value->IsTheHole(isolate)) { > > return bailout; > > } > > Since it is a dictionary iteration, value cannot be a hole. We should check if > the new key (pos) is the hole or not. So, I can bail out in two positions like > line 16828 and 16839 did in the original code if we don't want to refactor this > code. Hmm. The duplication doesn't look great either, and both look very tricky. I'm not sure it's worth tinkering with this code just to support certain cases that will work. What if we just bail out as soon as we see the object is non-extensible, like we do for sloppy arguments? That would work just as well, right?
On 2017/02/08 01:14:07, adamk wrote: > https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc > File src/objects.cc (right): > > https://codereview.chromium.org/2664173002/diff/80001/src/objects.cc#newcode1... > src/objects.cc:16817: Handle<Object> value(dict->ValueAt(i), isolate); > On 2017/02/07 09:45:49, tunz wrote: > > On 2017/02/07 00:23:46, adamk wrote: > > > Rather than changing the logic below, couldn't you simply bail out here if > > > you've found a hole? > > > > > > if (value->IsTheHole(isolate)) { > > > return bailout; > > > } > > > > Since it is a dictionary iteration, value cannot be a hole. We should check if > > the new key (pos) is the hole or not. So, I can bail out in two positions like > > line 16828 and 16839 did in the original code if we don't want to refactor > this > > code. > > Hmm. The duplication doesn't look great either, and both look very tricky. > > I'm not sure it's worth tinkering with this code just to support certain cases > that will work. What if we just bail out as soon as we see the object is > non-extensible, like we do for sloppy arguments? That would work just as well, > right? Yes, we can do that. Then, simply lead non-extensible object to SafeRemoveArrayHoles.
lgtm Thanks for working through the back-and-forth, I think this simpler approach makes sense at least without some evidence that this is a case that needs optimization (the fact that all non-extensible arrays already have a dictionary backing store suggests that performance is likely not super-important in this case).
The CQ bit was checked by cwhan.tunz@gmail.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.
On 2017/02/10 19:04:32, adamk wrote: > lgtm > > Thanks for working through the back-and-forth, I think this simpler approach > makes sense at least without some evidence that this is a case that needs > optimization (the fact that all non-extensible arrays already have a dictionary > backing store suggests that performance is likely not super-important in this > case). Yes, I agree. Thanks!
The CQ bit was checked by cwhan.tunz@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486817955560510,
"parent_rev": "c9950faf47edc34f4ee7e951a6ceeed647ce6ab9", "commit_rev":
"48dff523f7348af657d610f789d326d85f84ecec"}
Message was sent while issue was closed.
Description was changed from ========== Throw when a holey property is set in Array.sort Do not allow that holey properties are defined in Array sort. Throw a type error if the array is not extensible and there are holey properties in the middle of the array. BUG=v8:4888 ========== to ========== Throw when a holey property is set in Array.sort Do not allow that holey properties are defined in Array sort. Throw a type error if the array is not extensible and there are holey properties in the middle of the array. BUG=v8:4888 Review-Url: https://codereview.chromium.org/2664173002 Cr-Commit-Position: refs/heads/master@{#43126} Committed: https://chromium.googlesource.com/v8/v8/+/48dff523f7348af657d610f789d326d85f8... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/48dff523f7348af657d610f789d326d85f8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
