Commit 772b7730 authored by darin@apple.com's avatar darin@apple.com

2008-06-11 Darin Adler <darin@apple.com>

        Reviewed by Alexey.

        - fix https://bugs.webkit.org/show_bug.cgi?id=19442
          JavaScript array implementation doesn't maintain m_numValuesInVector when sorting

        * kjs/array_instance.cpp:
        (KJS::ArrayInstance::checkConsistency): Added. Empty inline version for when
        consistency checks are turned off.
        (KJS::ArrayInstance::ArrayInstance): Check consistency after construction.
        (KJS::ArrayInstance::~ArrayInstance): Check consistency before destruction.
        (KJS::ArrayInstance::put): Check consistency before and after.
        (KJS::ArrayInstance::deleteProperty): Ditto.
        (KJS::ArrayInstance::setLength): Ditto.
        (KJS::compareByStringPairForQSort): Use typedef for clarity.
        (KJS::ArrayInstance::sort): Check consistency before and after. Also broke the loop
        to set up sorting into two separate passes. Added FIXMEs about various exception
        safety issues. Added code to set m_numValuesInVector after sorting.
        (KJS::ArrayInstance::compactForSorting): Ditto.

        * kjs/array_instance.h: Added a definition of an enum for the types of consistency
        check and a declaration of the consistency checking function.



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@34496 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 312e5395
2008-06-11 Darin Adler <darin@apple.com>
Reviewed by Alexey.
- fix https://bugs.webkit.org/show_bug.cgi?id=19442
JavaScript array implementation doesn't maintain m_numValuesInVector when sorting
* kjs/array_instance.cpp:
(KJS::ArrayInstance::checkConsistency): Added. Empty inline version for when
consistency checks are turned off.
(KJS::ArrayInstance::ArrayInstance): Check consistency after construction.
(KJS::ArrayInstance::~ArrayInstance): Check consistency before destruction.
(KJS::ArrayInstance::put): Check consistency before and after.
(KJS::ArrayInstance::deleteProperty): Ditto.
(KJS::ArrayInstance::setLength): Ditto.
(KJS::compareByStringPairForQSort): Use typedef for clarity.
(KJS::ArrayInstance::sort): Check consistency before and after. Also broke the loop
to set up sorting into two separate passes. Added FIXMEs about various exception
safety issues. Added code to set m_numValuesInVector after sorting.
(KJS::ArrayInstance::compactForSorting): Ditto.
* kjs/array_instance.h: Added a definition of an enum for the types of consistency
check and a declaration of the consistency checking function.
2008-06-10 Kevin Ollivier <kevino@theolliviers.com>
wx build fix. Link against libedit on Mac since HAVE(READLINE) is defined there.
......
......@@ -27,6 +27,8 @@
#include <wtf/Assertions.h>
#include <wtf/AVLTree.h>
#define CHECK_ARRAY_CONSISTENCY 0
using namespace std;
namespace KJS {
......@@ -40,7 +42,7 @@ struct ArrayStorage {
JSValue* m_vector[1];
};
// 0xFFFFFFFF is a bit weird -- is not an array index even though it's an integer
// 0xFFFFFFFF is a bit weird -- is not an array index even though it's an integer.
static const unsigned maxArrayIndex = 0xFFFFFFFEU;
// Our policy for when to use a vector and when to use a sparse map.
......@@ -69,6 +71,14 @@ static inline bool isDenseEnoughForVector(unsigned length, unsigned numValues)
return length / minDensityMultiplier <= numValues;
}
#if !CHECK_ARRAY_CONSISTENCY
inline void ArrayInstance::checkConsistency(ConsistencyCheckType)
{
}
#endif
ArrayInstance::ArrayInstance(JSObject* prototype, unsigned initialLength)
: JSObject(prototype)
{
......@@ -79,6 +89,8 @@ ArrayInstance::ArrayInstance(JSObject* prototype, unsigned initialLength)
m_storage = static_cast<ArrayStorage*>(fastZeroedMalloc(storageSize(initialCapacity)));
Collector::reportExtraMemoryCost(initialCapacity * sizeof(JSValue*));
checkConsistency();
}
ArrayInstance::ArrayInstance(JSObject* prototype, const List& list)
......@@ -103,10 +115,14 @@ ArrayInstance::ArrayInstance(JSObject* prototype, const List& list)
// When the array is created non-empty, its cells are filled, so it's really no worse than
// a property map. Therefore don't report extra memory cost.
checkConsistency();
}
ArrayInstance::~ArrayInstance()
{
checkConsistency(DestructorConsistencyCheck);
delete m_storage->m_sparseValueMap;
fastFree(m_storage);
}
......@@ -209,6 +225,8 @@ void ArrayInstance::put(ExecState* exec, const Identifier& propertyName, JSValue
void ArrayInstance::put(ExecState* exec, unsigned i, JSValue* value)
{
checkConsistency();
unsigned length = m_length;
if (i >= length) {
if (i > maxArrayIndex) {
......@@ -225,6 +243,7 @@ void ArrayInstance::put(ExecState* exec, unsigned i, JSValue* value)
JSValue*& valueSlot = storage->m_vector[i];
storage->m_numValuesInVector += !valueSlot;
valueSlot = value;
checkConsistency();
return;
}
......@@ -250,6 +269,7 @@ void ArrayInstance::put(ExecState* exec, unsigned i, JSValue* value)
storage = m_storage;
++storage->m_numValuesInVector;
storage->m_vector[i] = value;
checkConsistency();
return;
}
......@@ -294,6 +314,8 @@ void ArrayInstance::put(ExecState* exec, unsigned i, JSValue* value)
storage->m_numValuesInVector = newNumValuesInVector;
m_storage = storage;
checkConsistency();
}
bool ArrayInstance::deleteProperty(ExecState* exec, const Identifier& propertyName)
......@@ -311,6 +333,8 @@ bool ArrayInstance::deleteProperty(ExecState* exec, const Identifier& propertyNa
bool ArrayInstance::deleteProperty(ExecState* exec, unsigned i)
{
checkConsistency();
ArrayStorage* storage = m_storage;
if (i < m_vectorLength) {
......@@ -318,6 +342,7 @@ bool ArrayInstance::deleteProperty(ExecState* exec, unsigned i)
bool hadValue = valueSlot;
valueSlot = 0;
storage->m_numValuesInVector -= hadValue;
checkConsistency();
return hadValue;
}
......@@ -326,11 +351,14 @@ bool ArrayInstance::deleteProperty(ExecState* exec, unsigned i)
SparseArrayValueMap::iterator it = map->find(i);
if (it != map->end()) {
map->remove(it);
checkConsistency();
return true;
}
}
}
checkConsistency();
if (i > maxArrayIndex)
return deleteProperty(exec, Identifier::from(i));
......@@ -340,7 +368,8 @@ bool ArrayInstance::deleteProperty(ExecState* exec, unsigned i)
void ArrayInstance::getPropertyNames(ExecState* exec, PropertyNameArray& propertyNames)
{
// FIXME: Filling PropertyNameArray with an identifier for every integer
// is incredibly inefficient for large arrays. We need a different approach.
// is incredibly inefficient for large arrays. We need a different approach,
// which almost certainly means a different structure for PropertyNameArray.
ArrayStorage* storage = m_storage;
......@@ -385,6 +414,8 @@ bool ArrayInstance::increaseVectorLength(unsigned newLength)
void ArrayInstance::setLength(unsigned newLength)
{
checkConsistency();
ArrayStorage* storage = m_storage;
unsigned length = m_length;
......@@ -413,6 +444,8 @@ void ArrayInstance::setLength(unsigned newLength)
}
m_length = newLength;
checkConsistency();
}
void ArrayInstance::mark()
......@@ -438,10 +471,12 @@ void ArrayInstance::mark()
}
}
typedef std::pair<JSValue*, UString> ArrayQSortPair;
static int compareByStringPairForQSort(const void* a, const void* b)
{
const std::pair<JSValue*, UString>* va = static_cast<const std::pair<JSValue*, UString>*>(a);
const std::pair<JSValue*, UString>* vb = static_cast<const std::pair<JSValue*, UString>*>(b);
const ArrayQSortPair* va = static_cast<const ArrayQSortPair*>(a);
const ArrayQSortPair* vb = static_cast<const ArrayQSortPair*>(b);
return compare(va->second, vb->second);
}
......@@ -461,7 +496,7 @@ void ArrayInstance::sort(ExecState* exec)
// buffer. Besides, this protects us from crashing if some objects have custom toString methods that return
// random or otherwise changing results, effectively making compare function inconsistent.
Vector<std::pair<JSValue*, UString> > values(lengthNotIncludingUndefined);
Vector<ArrayQSortPair> values(lengthNotIncludingUndefined);
if (!values.begin()) {
exec->setException(Error::create(exec, GeneralError, "Out of memory"));
return;
......@@ -471,9 +506,17 @@ void ArrayInstance::sort(ExecState* exec)
JSValue* value = m_storage->m_vector[i];
ASSERT(!value->isUndefined());
values[i].first = value;
values[i].second = value->toString(exec);
}
// FIXME: While calling these toString functions, the array could be mutated.
// In that case, objects pointed to by values in this vector might get garbage-collected!
// FIXME: The following loop continues to call toString on subsequent values even after
// a toString call raises an exception.
for (size_t i = 0; i < lengthNotIncludingUndefined; i++)
values[i].second = values[i].first->toString(exec);
if (exec->hadException())
return;
......@@ -481,14 +524,20 @@ void ArrayInstance::sort(ExecState* exec)
// than O(N log N).
#if HAVE(MERGESORT)
mergesort(values.begin(), values.size(), sizeof(std::pair<JSValue*, UString>), compareByStringPairForQSort);
mergesort(values.begin(), values.size(), sizeof(ArrayQSortPair), compareByStringPairForQSort);
#else
// FIXME: QSort may not be stable in some implementations. ECMAScript-262 does not require this, but in practice, browsers perform stable sort.
qsort(values.begin(), values.size(), sizeof(std::pair<JSValue*, UString>), compareByStringPairForQSort);
// FIXME: The qsort library function is likely to not be a stable sort.
// ECMAScript-262 does not specify a stable sort, but in practice, browsers perform a stable sort.
qsort(values.begin(), values.size(), sizeof(ArrayQSortPair), compareByStringPairForQSort);
#endif
// FIXME: If the toString function changed the length of the array, this might be
// modifying the vector incorrectly.
for (size_t i = 0; i < lengthNotIncludingUndefined; i++)
m_storage->m_vector[i] = values[i].first;
checkConsistency(SortConsistencyCheck);
}
struct AVLTreeNodeForArrayCompare {
......@@ -558,6 +607,10 @@ struct AVLTreeAbstractorForArrayCompare {
void ArrayInstance::sort(ExecState* exec, JSObject* compareFunction)
{
checkConsistency();
// 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(m_length <= static_cast<unsigned>(std::numeric_limits<int>::max()));
......@@ -580,6 +633,9 @@ void ArrayInstance::sort(ExecState* exec, JSObject* compareFunction)
return;
}
// FIXME: If the compare function modifies the array, the vector, map, etc. could be modified
// right out from under us while we're building the tree here.
unsigned numDefined = 0;
unsigned numUndefined = 0;
......@@ -627,6 +683,9 @@ void ArrayInstance::sort(ExecState* exec, JSObject* compareFunction)
ASSERT(tree.abstractor().m_nodes.size() >= numDefined);
// FIXME: If the compare function changed the length of the array, the following might be
// modifying the vector incorrectly.
// Copy the values back into m_storage.
AVLTree<AVLTreeAbstractorForArrayCompare, 44>::Iterator iter;
iter.start_iter_least(tree);
......@@ -642,10 +701,16 @@ void ArrayInstance::sort(ExecState* exec, JSObject* compareFunction)
// Ensure that unused values in the vector are zeroed out.
for (unsigned i = newUsedVectorLength; i < usedVectorLength; ++i)
m_storage->m_vector[i] = 0;
m_storage->m_numValuesInVector = newUsedVectorLength;
checkConsistency(SortConsistencyCheck);
}
unsigned ArrayInstance::compactForSorting()
{
checkConsistency();
ArrayStorage* storage = m_storage;
unsigned usedVectorLength = min(m_length, m_vectorLength);
......@@ -690,6 +755,10 @@ unsigned ArrayInstance::compactForSorting()
for (unsigned i = newUsedVectorLength; i < usedVectorLength; ++i)
storage->m_vector[i] = 0;
storage->m_numValuesInVector = newUsedVectorLength;
checkConsistency(SortConsistencyCheck);
return numDefined;
}
......@@ -703,4 +772,42 @@ void ArrayInstance::setLazyCreationData(void* d)
m_storage->lazyCreationData = d;
}
#if CHECK_ARRAY_CONSISTENCY
void ArrayInstance::checkConsistency(ConsistencyCheckType type)
{
ASSERT(m_storage);
if (type == SortConsistencyCheck)
ASSERT(!m_storage->m_sparseValueMap);
unsigned numValuesInVector = 0;
for (unsigned i = 0; i < m_vectorLength; ++i) {
if (JSValue* value = m_storage->m_vector[i]) {
ASSERT(i < m_length);
if (type != DestructorConsistencyCheck)
value->type(); // Likely to crash if the object was deallocated.
++numValuesInVector;
} else {
if (type == SortConsistencyCheck)
ASSERT(i >= m_storage->m_numValuesInVector);
}
}
ASSERT(numValuesInVector == m_storage->m_numValuesInVector);
if (m_storage->m_sparseValueMap) {
SparseArrayValueMap::iterator end = m_storage->m_sparseValueMap->end();
for (SparseArrayValueMap::iterator it = m_storage->m_sparseValueMap->begin(); it != end; ++it) {
unsigned index = it->first;
ASSERT(index < m_length);
ASSERT(index >= m_vectorLength);
ASSERT(index <= maxArrayIndex);
ASSERT(it->second);
if (type != DestructorConsistencyCheck)
it->second->type(); // Likely to crash if the object was deallocated.
}
}
}
#endif
}
// -*- c-basic-offset: 2 -*-
/*
* Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
* Copyright (C) 2003, 2007 Apple Inc. All rights reserved.
* Copyright (C) 2003, 2007, 2008 Apple Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
......@@ -64,7 +64,10 @@ namespace KJS {
void setLength(unsigned);
bool increaseVectorLength(unsigned newLength);
unsigned compactForSorting();
unsigned compactForSorting();
enum ConsistencyCheckType { NormalConsistencyCheck, DestructorConsistencyCheck, SortConsistencyCheck };
void checkConsistency(ConsistencyCheckType = NormalConsistencyCheck);
unsigned m_length;
unsigned m_vectorLength;
......
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