diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 584359780f4c6c378beaec66e4ca1cd0a60f4f21..3539221b3d7f0f2e7dfc472cf606528da064c20b 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,16 @@ +2012-10-01 Filip Pizlo + + Address a FIXME in JSArray::sort + https://bugs.webkit.org/show_bug.cgi?id=98080 + + + Reviewed by Oliver Hunt. + + * fast/js/jsc-test-list: + * fast/js/script-tests/sort-with-side-effecting-comparisons.js: Added. + * fast/js/sort-with-side-effecting-comparisons-expected.txt: Added. + * fast/js/sort-with-side-effecting-comparisons.html: Added. + 2012-10-01 Roger Fong Unreviewed. Skipping flaky test on Windows: inspector/debugger/dynamic-script-tag.html. diff --git a/LayoutTests/fast/js/jsc-test-list b/LayoutTests/fast/js/jsc-test-list index 5fde28bfa65fae583b91396a54a97043096bbb03..571eeb39555635bf03cdc93d666bfc6f1dab7f05 100644 --- a/LayoutTests/fast/js/jsc-test-list +++ b/LayoutTests/fast/js/jsc-test-list @@ -297,6 +297,7 @@ fast/js/sort-no-jit-code-crash fast/js/sort-non-numbers fast/js/sort-randomly fast/js/sort-stability +fast/js/sort-with-side-effecting-comparisons fast/js/sparse-array fast/js/stack-overflow-arrity-catch fast/js/stack-overflow-catch diff --git a/LayoutTests/fast/js/script-tests/sort-with-side-effecting-comparisons.js b/LayoutTests/fast/js/script-tests/sort-with-side-effecting-comparisons.js new file mode 100644 index 0000000000000000000000000000000000000000..63a35ec595ff9a53262391b3924b5b02a910084d --- /dev/null +++ b/LayoutTests/fast/js/script-tests/sort-with-side-effecting-comparisons.js @@ -0,0 +1,21 @@ +description( +"Checks that sorting an array with a side-effecting comparison function doesn't trigger assertions." +); + +var array = []; + +for (var i = 0; i < 20000; ++i) + array.push(i); + +array.sort(function(a, b) { + array.shift(); + if (a < b) + return -1; + if (a > b) + return 1; + return 0; +}); + +testPassed("It worked."); + + diff --git a/LayoutTests/fast/js/sort-with-side-effecting-comparisons-expected.txt b/LayoutTests/fast/js/sort-with-side-effecting-comparisons-expected.txt new file mode 100644 index 0000000000000000000000000000000000000000..a0cff269ea432f664c3b175032432f9deae01fe2 --- /dev/null +++ b/LayoutTests/fast/js/sort-with-side-effecting-comparisons-expected.txt @@ -0,0 +1,10 @@ +Checks that sorting an array with a side-effecting comparison function doesn't trigger assertions. + +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". + + +PASS It worked. +PASS successfullyParsed is true + +TEST COMPLETE + diff --git a/LayoutTests/fast/js/sort-with-side-effecting-comparisons.html b/LayoutTests/fast/js/sort-with-side-effecting-comparisons.html new file mode 100644 index 0000000000000000000000000000000000000000..3a45a30222f0278831b06b494f7a21819534c657 --- /dev/null +++ b/LayoutTests/fast/js/sort-with-side-effecting-comparisons.html @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index c4b961501ad14b58069dcf72d6eb56b61f4e50a3..50016f9b8190757ba21821cf7eb5b7ac97d9c8de 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,27 @@ +2012-10-01 Filip Pizlo + + Address a FIXME in JSArray::sort + https://bugs.webkit.org/show_bug.cgi?id=98080 + + + Reviewed by Oliver Hunt. + + Get rid of fast sorting of sparse maps. I don't know that it's broken but I do know that we don't + have coverage for it. Then also address the FIXME in JSArray::sort regarding side-effecting + compare functions. + + * runtime/ArrayPrototype.cpp: + (JSC::arrayProtoFuncSort): + * runtime/JSArray.cpp: + (JSC::JSArray::sortNumeric): + (JSC::JSArray::sort): + (JSC::JSArray::compactForSorting): + * runtime/JSArray.h: + (JSArray): + * runtime/JSObject.h: + (JSC::JSObject::hasSparseMap): + (JSObject): + 2012-10-01 Jonathan Liu Remove unused sys/mman.h include diff --git a/Source/JavaScriptCore/runtime/ArrayPrototype.cpp b/Source/JavaScriptCore/runtime/ArrayPrototype.cpp index e1998e74d101780754301064a1f7e38869a9d547..2e23792140e621348ecea435806b926f179deb04 100644 --- a/Source/JavaScriptCore/runtime/ArrayPrototype.cpp +++ b/Source/JavaScriptCore/runtime/ArrayPrototype.cpp @@ -655,7 +655,7 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncSort(ExecState* exec) CallData callData; CallType callType = getCallData(function, callData); - if (thisObj->classInfo() == &JSArray::s_info && !asArray(thisObj)->inSparseIndexingMode() && !shouldUseSlowPut(thisObj->structure()->indexingType())) { + if (thisObj->classInfo() == &JSArray::s_info && !asArray(thisObj)->hasSparseMap() && !shouldUseSlowPut(thisObj->structure()->indexingType())) { if (isNumericCompareFunction(exec, callType, callData)) asArray(thisObj)->sortNumeric(exec, function, callType, callData); else if (callType != CallTypeNone) diff --git a/Source/JavaScriptCore/runtime/JSArray.cpp b/Source/JavaScriptCore/runtime/JSArray.cpp index 56f365c8da1fe37c7f1b280ac6d75654237d65c2..11eef23ec466bef816747fc3ade5a3226eea2e93 100644 --- a/Source/JavaScriptCore/runtime/JSArray.cpp +++ b/Source/JavaScriptCore/runtime/JSArray.cpp @@ -601,13 +601,10 @@ void JSArray::sortNumeric(ExecState* exec, JSValue compareFunction, CallType cal return; case ArrayWithArrayStorage: { - unsigned lengthNotIncludingUndefined = compactForSorting(exec->globalData()); + unsigned lengthNotIncludingUndefined = compactForSorting(); ArrayStorage* storage = m_butterfly->arrayStorage(); - - if (storage->m_sparseMap.get()) { - throwOutOfMemoryError(exec); - return; - } + + ASSERT(!storage->m_sparseMap); if (!lengthNotIncludingUndefined) return; @@ -646,12 +643,9 @@ void JSArray::sort(ExecState* exec) return; case ArrayWithArrayStorage: { - unsigned lengthNotIncludingUndefined = compactForSorting(exec->globalData()); + unsigned lengthNotIncludingUndefined = compactForSorting(); ArrayStorage* storage = m_butterfly->arrayStorage(); - if (storage->m_sparseMap.get()) { - throwOutOfMemoryError(exec); - return; - } + ASSERT(!storage->m_sparseMap); if (!lengthNotIncludingUndefined) return; @@ -811,18 +805,17 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType, return; case ArrayWithArrayStorage: { - ArrayStorage* storage = m_butterfly->arrayStorage(); - // FIXME: This ignores exceptions raised in the compare function or in toNumber. // The maximum tree depth is compiled in - but the caller is clearly up to no good // if a larger array is passed. - ASSERT(storage->length() <= static_cast(std::numeric_limits::max())); - if (storage->length() > static_cast(std::numeric_limits::max())) + ASSERT(arrayStorage()->length() <= static_cast(std::numeric_limits::max())); + if (arrayStorage()->length() > static_cast(std::numeric_limits::max())) return; - unsigned usedVectorLength = min(storage->length(), storage->vectorLength()); - unsigned nodeCount = usedVectorLength + (storage->m_sparseMap ? storage->m_sparseMap->size() : 0); + unsigned usedVectorLength = min(arrayStorage()->length(), arrayStorage()->vectorLength()); + ASSERT(!arrayStorage()->m_sparseMap); + unsigned nodeCount = usedVectorLength; if (!nodeCount) return; @@ -850,14 +843,18 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType, // Iterate over the array, ignoring missing values, counting undefined ones, and inserting all other ones into the tree. for (; numDefined < usedVectorLength; ++numDefined) { - JSValue v = storage->m_vector[numDefined].get(); + if (numDefined > arrayStorage()->vectorLength()) + break; + JSValue v = arrayStorage()->m_vector[numDefined].get(); if (!v || v.isUndefined()) break; tree.abstractor().m_nodes[numDefined].value = v; tree.insert(numDefined); } for (unsigned i = numDefined; i < usedVectorLength; ++i) { - JSValue v = storage->m_vector[i].get(); + if (i > arrayStorage()->vectorLength()) + break; + JSValue v = arrayStorage()->m_vector[i].get(); if (v) { if (v.isUndefined()) ++numUndefined; @@ -871,51 +868,34 @@ void JSArray::sort(ExecState* exec, JSValue compareFunction, CallType callType, unsigned newUsedVectorLength = numDefined + numUndefined; - if (SparseArrayValueMap* map = storage->m_sparseMap.get()) { - newUsedVectorLength += map->size(); - if (newUsedVectorLength > storage->vectorLength()) { - // Check that it is possible to allocate an array large enough to hold all the entries. - if ((newUsedVectorLength > MAX_STORAGE_VECTOR_LENGTH) || !increaseVectorLength(exec->globalData(), newUsedVectorLength)) { - throwOutOfMemoryError(exec); - return; - } - storage = m_butterfly->arrayStorage(); - } - - SparseArrayValueMap::const_iterator end = map->end(); - for (SparseArrayValueMap::const_iterator it = map->begin(); it != end; ++it) { - tree.abstractor().m_nodes[numDefined].value = it->second.getNonSparseMode(); - tree.insert(numDefined); - ++numDefined; - } - - deallocateSparseIndexMap(); - } + ASSERT(!arrayStorage()->m_sparseMap); - ASSERT(tree.abstractor().m_nodes.size() >= numDefined); + // The array size may have changed. Figure out the new bounds. + unsigned newestUsedVectorLength = min(arrayStorage()->length(), arrayStorage()->vectorLength()); - // FIXME: If the compare function changed the length of the array, the following might be - // modifying the vector incorrectly. + unsigned elementsToExtractThreshold = min(min(newestUsedVectorLength, numDefined), static_cast(tree.abstractor().m_nodes.size())); + unsigned undefinedElementsThreshold = min(newestUsedVectorLength, newUsedVectorLength); + unsigned clearElementsThreshold = min(newestUsedVectorLength, usedVectorLength); // Copy the values back into m_storage. AVLTree::Iterator iter; iter.start_iter_least(tree); JSGlobalData& globalData = exec->globalData(); - for (unsigned i = 0; i < numDefined; ++i) { - storage->m_vector[i].set(globalData, this, tree.abstractor().m_nodes[*iter].value); + for (unsigned i = 0; i < elementsToExtractThreshold; ++i) { + arrayStorage()->m_vector[i].set(globalData, this, tree.abstractor().m_nodes[*iter].value); ++iter; } // Put undefined values back in. - for (unsigned i = numDefined; i < newUsedVectorLength; ++i) - storage->m_vector[i].setUndefined(); - + for (unsigned i = elementsToExtractThreshold; i < undefinedElementsThreshold; ++i) + arrayStorage()->m_vector[i].setUndefined(); + // Ensure that unused values in the vector are zeroed out. - for (unsigned i = newUsedVectorLength; i < usedVectorLength; ++i) - storage->m_vector[i].clear(); - - storage->m_numValuesInVector = newUsedVectorLength; + for (unsigned i = undefinedElementsThreshold; i < clearElementsThreshold; ++i) + arrayStorage()->m_vector[i].clear(); + arrayStorage()->m_numValuesInVector = undefinedElementsThreshold; + return; } @@ -982,7 +962,7 @@ void JSArray::copyToArguments(ExecState* exec, CallFrame* callFrame, uint32_t le } } -unsigned JSArray::compactForSorting(JSGlobalData& globalData) +unsigned JSArray::compactForSorting() { ASSERT(!inSparseIndexingMode()); @@ -1016,23 +996,7 @@ unsigned JSArray::compactForSorting(JSGlobalData& globalData) unsigned newUsedVectorLength = numDefined + numUndefined; - if (SparseArrayValueMap* map = storage->m_sparseMap.get()) { - newUsedVectorLength += map->size(); - if (newUsedVectorLength > storage->vectorLength()) { - // Check that it is possible to allocate an array large enough to hold all the entries - if not, - // exception is thrown by caller. - if ((newUsedVectorLength > MAX_STORAGE_VECTOR_LENGTH) || !increaseVectorLength(globalData, newUsedVectorLength)) - return 0; - - storage = m_butterfly->arrayStorage(); - } - - SparseArrayValueMap::const_iterator end = map->end(); - for (SparseArrayValueMap::const_iterator it = map->begin(); it != end; ++it) - storage->m_vector[numDefined++].setWithoutWriteBarrier(it->second.getNonSparseMode()); - - deallocateSparseIndexMap(); - } + ASSERT(!storage->m_sparseMap); for (unsigned i = numDefined; i < newUsedVectorLength; ++i) storage->m_vector[i].setUndefined(); diff --git a/Source/JavaScriptCore/runtime/JSArray.h b/Source/JavaScriptCore/runtime/JSArray.h index 179255ec9bb3913490accf81b12832cacda49e1a..5a1507e0de644d0f4e715aee3900d73f94604e0b 100644 --- a/Source/JavaScriptCore/runtime/JSArray.h +++ b/Source/JavaScriptCore/runtime/JSArray.h @@ -103,7 +103,7 @@ namespace JSC { bool unshiftCountSlowCase(JSGlobalData&, bool, unsigned); - unsigned compactForSorting(JSGlobalData&); + unsigned compactForSorting(); }; inline Butterfly* createArrayButterfly(JSGlobalData& globalData, unsigned initialLength) diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h index 59fef7ad98563f5721d57cb04aaeb06e324be36a..c9bf791cf88dfbbe03568008ecc201d53c1a71b7 100644 --- a/Source/JavaScriptCore/runtime/JSObject.h +++ b/Source/JavaScriptCore/runtime/JSObject.h @@ -327,6 +327,19 @@ namespace JSC { } } + bool hasSparseMap() + { + switch (structure()->indexingType()) { + case ALL_BLANK_INDEXING_TYPES: + return false; + case ALL_ARRAY_STORAGE_INDEXING_TYPES: + return m_butterfly->arrayStorage()->m_sparseMap; + default: + ASSERT_NOT_REACHED(); + return false; + } + } + bool inSparseIndexingMode() { switch (structure()->indexingType()) {