Commit 40fcdef2 authored by mhahnenberg@apple.com's avatar mhahnenberg@apple.com

(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.

Source/JavaScriptCore:

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):

LayoutTests:

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.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@156214 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 79d746c3
2013-09-20 Mark Hahnenberg <mhahnenberg@apple.com>
(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 <ap@apple.com>
Fix test after <http://trac.webkit.org/changeset/156203>
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
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<script src="../resources/js-test-pre.js"></script>
</head>
<body>
<script src="script-tests/array-splice-with-holes.js"></script>
<script src="../resources/js-test-post.js"></script>
</body>
</html>
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");
2013-09-20 Mark Hahnenberg <mhahnenberg@apple.com>
(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 <fpizlo@apple.com>
REGRESSION(r156047): WebCore hangs inside JSC::toInt32(double)
......
......@@ -768,21 +768,21 @@ bool JSArray::shiftCountWithAnyIndexingType(ExecState* exec, unsigned startIndex
// 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 @@ bool JSArray::shiftCountWithAnyIndexingType(ExecState* exec, unsigned startIndex
// 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 @@ bool JSArray::unshiftCountWithAnyIndexingType(ExecState* exec, unsigned startInd
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 @@ bool JSArray::unshiftCountWithAnyIndexingType(ExecState* exec, unsigned startInd
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;
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment