Diff
Modified: trunk/LayoutTests/ChangeLog (156213 => 156214)
--- trunk/LayoutTests/ChangeLog 2013-09-21 00:07:32 UTC (rev 156213)
+++ trunk/LayoutTests/ChangeLog 2013-09-21 00:24:08 UTC (rev 156214)
@@ -1,3 +1,16 @@
+2013-09-20 Mark Hahnenberg <[email protected]>
+
+ (un)shiftCountWithAnyIndexingType will start over in the middle of copying if it sees a hole
+ https://bugs.webkit.org/show_bug.cgi?id=121717
+
+ Reviewed by Oliver Hunt.
+
+ Added test to make sure that splicing an array with holes works correctly.
+
+ * js/array-splice-with-holes-expected.txt: Added.
+ * js/array-splice-with-holes.html: Added.
+ * js/script-tests/array-splice-with-holes.js: Added.
+
2013-09-20 Alexey Proskuryakov <[email protected]>
Fix test after <http://trac.webkit.org/changeset/156203>
Added: trunk/LayoutTests/js/array-splice-with-holes-expected.txt (0 => 156214)
--- trunk/LayoutTests/js/array-splice-with-holes-expected.txt (rev 0)
+++ trunk/LayoutTests/js/array-splice-with-holes-expected.txt 2013-09-21 00:24:08 UTC (rev 156214)
@@ -0,0 +1,13 @@
+Test to ensure correct behaviour of Array.prototype.splice when the array has holes in it.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS actualArray.toString() is expectedArray.toString()
+PASS actualArray.length is expectedArray.length
+PASS actualArray.toString() is expectedArray.toString()
+PASS actualArray.length is expectedArray.length
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/js/array-splice-with-holes.html (0 => 156214)
--- trunk/LayoutTests/js/array-splice-with-holes.html (rev 0)
+++ trunk/LayoutTests/js/array-splice-with-holes.html 2013-09-21 00:24:08 UTC (rev 156214)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Added: trunk/LayoutTests/js/script-tests/array-splice-with-holes.js (0 => 156214)
--- trunk/LayoutTests/js/script-tests/array-splice-with-holes.js (rev 0)
+++ trunk/LayoutTests/js/script-tests/array-splice-with-holes.js 2013-09-21 00:24:08 UTC (rev 156214)
@@ -0,0 +1,27 @@
+description("Test to ensure correct behaviour of Array.prototype.splice when the array has holes in it.");
+
+var actualArray = new Array(20);
+var seedArray = ["Black","White","Blue","Red","Green","Orange","Purple","Cyan","Yellow"];
+for (var i = 0; i < seedArray.length; i++)
+ actualArray[i] = seedArray[i];
+actualArray.splice(3, 1);
+var expectedSeedArray = ["Black","White","Blue","Green","Orange","Purple","Cyan","Yellow"];
+var expectedArray = new Array(19);
+for (var i = 0; i < expectedSeedArray.length; i++)
+ expectedArray[i] = expectedSeedArray[i];
+
+shouldBe("actualArray.toString()", "expectedArray.toString()");
+shouldBe("actualArray.length", "expectedArray.length");
+
+actualArray = new Array(20);
+for (var i = 0; i < seedArray.length; i += 2)
+ actualArray[i] = seedArray[i];
+actualArray.splice(2, 2);
+var expectedArray = new Array(18);
+expectedArray[0] = "Black";
+expectedArray[2] = "Green";
+expectedArray[4] = "Purple";
+expectedArray[6] = "Yellow";
+
+shouldBe("actualArray.toString()", "expectedArray.toString()");
+shouldBe("actualArray.length", "expectedArray.length");
Modified: trunk/Source/_javascript_Core/ChangeLog (156213 => 156214)
--- trunk/Source/_javascript_Core/ChangeLog 2013-09-21 00:07:32 UTC (rev 156213)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-09-21 00:24:08 UTC (rev 156214)
@@ -1,3 +1,17 @@
+2013-09-20 Mark Hahnenberg <[email protected]>
+
+ (un)shiftCountWithAnyIndexingType will start over in the middle of copying if it sees a hole
+ https://bugs.webkit.org/show_bug.cgi?id=121717
+
+ Reviewed by Oliver Hunt.
+
+ This bug caused the array to become corrupted. We now check for holes before we start moving things,
+ and start moving things only once we've determined that there are none.
+
+ * runtime/JSArray.cpp:
+ (JSC::JSArray::shiftCountWithAnyIndexingType):
+ (JSC::JSArray::unshiftCountWithAnyIndexingType):
+
2013-09-20 Filip Pizlo <[email protected]>
REGRESSION(r156047): WebCore hangs inside JSC::toInt32(double)
Modified: trunk/Source/_javascript_Core/runtime/JSArray.cpp (156213 => 156214)
--- trunk/Source/_javascript_Core/runtime/JSArray.cpp 2013-09-21 00:07:32 UTC (rev 156213)
+++ trunk/Source/_javascript_Core/runtime/JSArray.cpp 2013-09-21 00:24:08 UTC (rev 156214)
@@ -768,21 +768,21 @@
// so only if it's not horribly slow.
if (oldLength - (startIndex + count) >= MIN_SPARSE_ARRAY_INDEX)
return shiftCountWithArrayStorage(startIndex, count, ensureArrayStorage(exec->vm()));
-
+
+ // Storing to a hole is fine since we're still having a good time. But reading from a hole
+ // is totally not fine, since we might have to read from the proto chain.
+ // We have to check for holes before we start moving things around so that we don't get halfway
+ // through shifting and then realize we should have been in ArrayStorage mode.
unsigned end = oldLength - count;
for (unsigned i = startIndex; i < end; ++i) {
- // Storing to a hole is fine since we're still having a good time. But reading
- // from a hole is totally not fine, since we might have to read from the proto
- // chain.
JSValue v = m_butterfly->contiguous()[i + count].get();
- if (UNLIKELY(!v)) {
- // The purpose of this path is to ensure that we don't make the same
- // mistake in the future: shiftCountWithArrayStorage() can't do anything
- // about holes (at least for now), but it can detect them quickly. So
- // we convert to array storage and then allow the array storage path to
- // figure it out.
+ if (UNLIKELY(!v))
return shiftCountWithArrayStorage(startIndex, count, ensureArrayStorage(exec->vm()));
- }
+ }
+
+ for (unsigned i = startIndex; i < end; ++i) {
+ JSValue v = m_butterfly->contiguous()[i + count].get();
+ ASSERT(v);
// No need for a barrier since we're just moving data around in the same vector.
// This is in line with our standing assumption that we won't have a deletion
// barrier.
@@ -803,21 +803,21 @@
// so only if it's not horribly slow.
if (oldLength - (startIndex + count) >= MIN_SPARSE_ARRAY_INDEX)
return shiftCountWithArrayStorage(startIndex, count, ensureArrayStorage(exec->vm()));
-
+
+ // Storing to a hole is fine since we're still having a good time. But reading from a hole
+ // is totally not fine, since we might have to read from the proto chain.
+ // We have to check for holes before we start moving things around so that we don't get halfway
+ // through shifting and then realize we should have been in ArrayStorage mode.
unsigned end = oldLength - count;
for (unsigned i = startIndex; i < end; ++i) {
- // Storing to a hole is fine since we're still having a good time. But reading
- // from a hole is totally not fine, since we might have to read from the proto
- // chain.
double v = m_butterfly->contiguousDouble()[i + count];
- if (UNLIKELY(v != v)) {
- // The purpose of this path is to ensure that we don't make the same
- // mistake in the future: shiftCountWithArrayStorage() can't do anything
- // about holes (at least for now), but it can detect them quickly. So
- // we convert to array storage and then allow the array storage path to
- // figure it out.
+ if (UNLIKELY(v != v))
return shiftCountWithArrayStorage(startIndex, count, ensureArrayStorage(exec->vm()));
- }
+ }
+
+ for (unsigned i = startIndex; i < end; ++i) {
+ double v = m_butterfly->contiguousDouble()[i + count];
+ ASSERT(v == v);
// No need for a barrier since we're just moving data around in the same vector.
// This is in line with our standing assumption that we won't have a deletion
// barrier.
@@ -902,11 +902,18 @@
return unshiftCountWithArrayStorage(exec, startIndex, count, ensureArrayStorage(exec->vm()));
ensureLength(exec->vm(), oldLength + count);
-
+
+ // We have to check for holes before we start moving things around so that we don't get halfway
+ // through shifting and then realize we should have been in ArrayStorage mode.
for (unsigned i = oldLength; i-- > startIndex;) {
JSValue v = m_butterfly->contiguous()[i].get();
if (UNLIKELY(!v))
return unshiftCountWithArrayStorage(exec, startIndex, count, ensureArrayStorage(exec->vm()));
+ }
+
+ for (unsigned i = oldLength; i-- > startIndex;) {
+ JSValue v = m_butterfly->contiguous()[i].get();
+ ASSERT(v);
m_butterfly->contiguous()[i + count].setWithoutWriteBarrier(v);
}
@@ -928,10 +935,17 @@
ensureLength(exec->vm(), oldLength + count);
+ // We have to check for holes before we start moving things around so that we don't get halfway
+ // through shifting and then realize we should have been in ArrayStorage mode.
for (unsigned i = oldLength; i-- > startIndex;) {
double v = m_butterfly->contiguousDouble()[i];
if (UNLIKELY(v != v))
return unshiftCountWithArrayStorage(exec, startIndex, count, ensureArrayStorage(exec->vm()));
+ }
+
+ for (unsigned i = oldLength; i-- > startIndex;) {
+ double v = m_butterfly->contiguousDouble()[i];
+ ASSERT(v == v);
m_butterfly->contiguousDouble()[i + count] = v;
}
