From b959ece4707852d213e4f8e43c5670f62d538f1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Z=C3=BCnd?= Date: Tue, 22 Jan 2019 16:53:37 +0100 Subject: [PATCH] [array] Enable copying from the prototype chain when sorting JSArrays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL enables the pre-processing step of copying from the prototype chain for JSArrays. Previously, this was done for everything BUT JSArrays. This brings Array#sort more in line with other engines in the case of undefined behavior. R=jgruber@chromium.org Bug: v8:8666 Change-Id: I832d470dc02111b64dc4919e84e7e3e47c8fdd47 Reviewed-on: https://chromium-review.googlesource.com/c/1426119 Commit-Queue: Simon Zünd Reviewed-by: Jakob Gruber Reviewed-by: Mathias Bynens Cr-Commit-Position: refs/heads/master@{#58999} --- src/runtime/runtime-array.cc | 9 ++++++--- test/mjsunit/array-sort.js | 21 +++++++++++++++++++++ test/webkit/array-holes-expected.txt | 2 +- test/webkit/array-holes.js | 2 +- 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/runtime/runtime-array.cc b/src/runtime/runtime-array.cc index 5346658798..d18ced02bd 100644 --- a/src/runtime/runtime-array.cc +++ b/src/runtime/runtime-array.cc @@ -393,10 +393,10 @@ RUNTIME_FUNCTION(Runtime_PrepareElementsForSort) { // Counter for sorting arrays that have non-packed elements and where either // the ElementsProtector is invalid or the prototype does not match // Array.prototype. + JSObject initial_array_proto = JSObject::cast( + isolate->native_context()->get(Context::INITIAL_ARRAY_PROTOTYPE_INDEX)); if (object->IsJSArray() && !Handle::cast(object)->HasFastPackedElements()) { - JSObject initial_array_proto = JSObject::cast( - isolate->native_context()->get(Context::INITIAL_ARRAY_PROTOTYPE_INDEX)); if (!isolate->IsNoElementsProtectorIntact() || object->map()->prototype() != initial_array_proto) { isolate->CountUsage( @@ -404,7 +404,10 @@ RUNTIME_FUNCTION(Runtime_PrepareElementsForSort) { } } - if (!object->IsJSArray()) { + // Skip copying from prototype for JSArrays with ElementsProtector intact and + // the original array prototype. + if (!object->IsJSArray() || !isolate->IsNoElementsProtectorIntact() || + object->map()->prototype() != initial_array_proto) { RETURN_FAILURE_ON_EXCEPTION(isolate, CopyFromPrototype(isolate, object, length)); } diff --git a/test/mjsunit/array-sort.js b/test/mjsunit/array-sort.js index 8c2c6fca63..ca0daadf04 100644 --- a/test/mjsunit/array-sort.js +++ b/test/mjsunit/array-sort.js @@ -567,6 +567,27 @@ function TestPrototypeHoles() { } TestPrototypeHoles(); +// The following test ensures that elements on the prototype are also copied +// for JSArrays and not only JSObjects. +function TestArrayPrototypeHasElements() { + let array = [1, 2, 3, 4, 5]; + for (let i = 0; i < array.length; i++) { + delete array[i]; + Object.prototype[i] = 42; + } + + let comparator_called = false; + array.sort(function (a, b) { + if (a === 42 || b === 42) { + comparator_called = true; + } + return a - b; + }); + + assertTrue(comparator_called); +} +TestArrayPrototypeHasElements(); + // The following Tests make sure that there is no crash when the element kind // or the array length changes. Since comparison functions like this are not // consistent, we do not have to make sure that the array is actually sorted diff --git a/test/webkit/array-holes-expected.txt b/test/webkit/array-holes-expected.txt index 05e095f386..992667f9ad 100644 --- a/test/webkit/array-holes-expected.txt +++ b/test/webkit/array-holes-expected.txt @@ -59,7 +59,7 @@ PASS showHoles([0, , 2].concat([3, , 5])) is '[0, peekaboo, 2, 3, peekaboo, 5]' PASS showHoles([0, , 2, 3].reverse()) is '[3, 2, peekaboo, 0]' PASS a = [0, , 2, 3]; a.shift(); showHoles(a) is '[peekaboo, 2, 3]' PASS showHoles([0, , 2, 3].slice(0, 3)) is '[0, peekaboo, 2]' -PASS showHoles([0, , 2, 3].sort()) is '[0, 2, 3, hole]' +PASS showHoles([0, , 2, 3].sort()) is '[0, 2, 3, peekaboo]' PASS showHoles([0, undefined, 2, 3].sort()) is '[0, 2, 3, undefined]' PASS a = [0, , 2, 3]; a.splice(2, 3, 5, 6); showHoles(a) is '[0, hole, 5, 6]' PASS a = [0, , 2, 3]; a.unshift(4); showHoles(a) is '[4, 0, peekaboo, 2, 3]' diff --git a/test/webkit/array-holes.js b/test/webkit/array-holes.js index 007626b847..182492c1c8 100644 --- a/test/webkit/array-holes.js +++ b/test/webkit/array-holes.js @@ -110,7 +110,7 @@ shouldBe("showHoles([0, , 2].concat([3, , 5]))", "'[0, peekaboo, 2, 3, peekaboo, shouldBe("showHoles([0, , 2, 3].reverse())", "'[3, 2, peekaboo, 0]'"); shouldBe("a = [0, , 2, 3]; a.shift(); showHoles(a)", "'[peekaboo, 2, 3]'"); shouldBe("showHoles([0, , 2, 3].slice(0, 3))", "'[0, peekaboo, 2]'"); -shouldBe("showHoles([0, , 2, 3].sort())", "'[0, 2, 3, hole]'"); +shouldBe("showHoles([0, , 2, 3].sort())", "'[0, 2, 3, peekaboo]'"); shouldBe("showHoles([0, undefined, 2, 3].sort())", "'[0, 2, 3, undefined]'"); shouldBe("a = [0, , 2, 3]; a.splice(2, 3, 5, 6); showHoles(a)", "'[0, hole, 5, 6]'"); shouldBe("a = [0, , 2, 3]; a.unshift(4); showHoles(a)", "'[4, 0, peekaboo, 2, 3]'");