https://bugs.webkit.org/show_bug.cgi?id=166874
Reviewed by Mark Lam.
This fixes two separate bugs, both of which I detected by running
array-splice-contiguous.js in extreme anger:
1) Some of the paths of shifting and unshifting were not grabbing the internal cell
lock. This was causing the array storage scan to crash, even though it was well
synchronized (the scan does hold the lock). The fix is just to hold the lock anywhere
that memmoves the innards of the butterfly.
2) Out of line property scanning was synchronized using double collect snapshot. Array
storage scanning was synchronized using locks. But what if array storage
transformations messed up the out of line properties? It turns out that we actually
need to hoist the array storage scanner's locking up into the double collect
snapshot.
I don't know how to write a test that does any better of a job of catching this than
array-splice-contiguous.js.
* heap/DeferGC.h: Make DisallowGC usable even if NDEBUG.
* runtime/JSArray.cpp:
(JSC::JSArray::unshiftCountSlowCase):
(JSC::JSArray::shiftCountWithArrayStorage):
(JSC::JSArray::unshiftCountWithArrayStorage):
* runtime/JSObject.cpp:
(JSC::JSObject::visitButterflyImpl):
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@210553
268f45cc-cd09-0410-ab3c-
d52691b4dbfc
+2017-01-09 Filip Pizlo <fpizlo@apple.com>
+
+ JSArray has some object scanning races
+ https://bugs.webkit.org/show_bug.cgi?id=166874
+
+ Reviewed by Mark Lam.
+
+ This fixes two separate bugs, both of which I detected by running
+ array-splice-contiguous.js in extreme anger:
+
+ 1) Some of the paths of shifting and unshifting were not grabbing the internal cell
+ lock. This was causing the array storage scan to crash, even though it was well
+ synchronized (the scan does hold the lock). The fix is just to hold the lock anywhere
+ that memmoves the innards of the butterfly.
+
+ 2) Out of line property scanning was synchronized using double collect snapshot. Array
+ storage scanning was synchronized using locks. But what if array storage
+ transformations messed up the out of line properties? It turns out that we actually
+ need to hoist the array storage scanner's locking up into the double collect
+ snapshot.
+
+ I don't know how to write a test that does any better of a job of catching this than
+ array-splice-contiguous.js.
+
+ * heap/DeferGC.h: Make DisallowGC usable even if NDEBUG.
+ * runtime/JSArray.cpp:
+ (JSC::JSArray::unshiftCountSlowCase):
+ (JSC::JSArray::shiftCountWithArrayStorage):
+ (JSC::JSArray::unshiftCountWithArrayStorage):
+ * runtime/JSObject.cpp:
+ (JSC::JSObject::visitButterflyImpl):
+
2017-01-10 Andreas Kling <akling@apple.com>
Crash when GC heap grows way too large.
/*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
Heap& m_heap;
};
-#ifndef NDEBUG
class DisallowGC {
WTF_MAKE_NONCOPYABLE(DisallowGC);
public:
DisallowGC()
{
+#ifndef NDEBUG
WTF::threadSpecificSet(s_isGCDisallowedOnCurrentThread, reinterpret_cast<void*>(true));
+#endif
}
~DisallowGC()
{
+#ifndef NDEBUG
WTF::threadSpecificSet(s_isGCDisallowedOnCurrentThread, reinterpret_cast<void*>(false));
+#endif
}
static bool isGCDisallowedOnCurrentThread()
{
+#ifndef NDEBUG
return !!WTF::threadSpecificGet(s_isGCDisallowedOnCurrentThread);
+#else
+ return false;
+#endif
}
static void initialize()
{
+#ifndef NDEBUG
WTF::threadSpecificKeyCreate(&s_isGCDisallowedOnCurrentThread, 0);
+#endif
}
+#ifndef NDEBUG
JS_EXPORT_PRIVATE static WTF::ThreadSpecificKey s_isGCDisallowedOnCurrentThread;
+#endif
};
-#endif // NDEBUG
} // namespace JSC
/*
* Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
- * Copyright (C) 2003, 2007-2009, 2012-2013, 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
* Copyright (C) 2003 Peter Kelly (pmk@post.com)
* Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
*
}
// This method makes room in the vector, but leaves the new space for count slots uncleared.
-bool JSArray::unshiftCountSlowCase(VM& vm, DeferGC&, bool addToFront, unsigned count)
+bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool addToFront, unsigned count)
{
ArrayStorage* storage = ensureArrayStorage(vm);
Butterfly* butterfly = storage->butterfly();
if (startIndex >= vectorLength)
return true;
+ DisallowGC disallowGC;
+ auto locker = holdLock(*this);
+
if (startIndex + count > vectorLength)
count = vectorLength - startIndex;
// the start of the Butterfly, which needs to point at the first indexed property in the used
// portion of the vector.
Butterfly* butterfly = m_butterfly.get()->shift(structure(), count);
- m_butterfly.setWithoutBarrier(butterfly);
+ setButterfly(vm, butterfly);
storage = butterfly->arrayStorage();
storage->m_indexBias += count;
setButterfly(vm, newButterfly);
} else if (!moveFront && vectorLength - length >= count)
storage = storage->butterfly()->arrayStorage();
- else if (unshiftCountSlowCase(vm, deferGC, moveFront, count))
+ else if (unshiftCountSlowCase(locker, vm, deferGC, moveFront, count))
storage = arrayStorage();
else {
throwOutOfMemoryError(exec, scope);
/*
* Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
- * Copyright (C) 2003, 2007, 2008, 2009, 2012, 2015-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2017 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
bool unshiftCountWithAnyIndexingType(ExecState*, unsigned startIndex, unsigned count);
bool unshiftCountWithArrayStorage(ExecState*, unsigned startIndex, unsigned count, ArrayStorage*);
- bool unshiftCountSlowCase(VM&, DeferGC&, bool, unsigned);
+ bool unshiftCountSlowCase(const AbstractLocker&, VM&, DeferGC&, bool, unsigned);
bool setLengthWithArrayStorage(ExecState*, unsigned newLength, bool throwException, ArrayStorage*);
void setLengthWritable(ExecState*, bool writable);
return nullptr;
structure = vm.getStructure(structureID);
lastOffset = structure->lastOffset();
+ IndexingType indexingType = structure->indexingType();
+ Locker<JSCell> locker(NoLockingNecessary);
+ switch (indexingType) {
+ case ALL_CONTIGUOUS_INDEXING_TYPES:
+ case ALL_ARRAY_STORAGE_INDEXING_TYPES:
+ // We need to hold this lock to protect against changes to the innards of the butterfly
+ // that can happen when the butterfly is used for array storage. We conservatively
+ // assume that a contiguous butterfly may transform into an array storage one, though
+ // this is probably more conservative than necessary.
+ locker = Locker<JSCell>(*this);
+ break;
+ default:
+ break;
+ }
WTF::loadLoadFence();
butterfly = this->butterfly();
if (!butterfly)
markAuxiliaryAndVisitOutOfLineProperties(visitor, butterfly, structure, lastOffset);
- IndexingType oldType = structure->indexingType();
- switch (oldType) {
+ ASSERT(indexingType == structure->indexingType());
+
+ switch (indexingType) {
case ALL_CONTIGUOUS_INDEXING_TYPES:
- case ALL_ARRAY_STORAGE_INDEXING_TYPES: {
- // This lock is here to protect Contiguous->ArrayStorage transitions, but we could make that
- // race work if we needed to.
- auto locker = holdLock(*this);
- IndexingType newType = this->indexingType();
- butterfly = this->butterfly();
- switch (newType) {
- case ALL_CONTIGUOUS_INDEXING_TYPES:
- visitor.appendValuesHidden(butterfly->contiguous().data(), butterfly->publicLength());
- break;
- default: // ALL_ARRAY_STORAGE_INDEXING_TYPES
- visitor.appendValuesHidden(butterfly->arrayStorage()->m_vector, butterfly->arrayStorage()->vectorLength());
- if (butterfly->arrayStorage()->m_sparseMap)
- visitor.append(butterfly->arrayStorage()->m_sparseMap);
- break;
- }
+ visitor.appendValuesHidden(butterfly->contiguous().data(), butterfly->publicLength());
+ break;
+ case ALL_ARRAY_STORAGE_INDEXING_TYPES:
+ visitor.appendValuesHidden(butterfly->arrayStorage()->m_vector, butterfly->arrayStorage()->vectorLength());
+ if (butterfly->arrayStorage()->m_sparseMap)
+ visitor.append(butterfly->arrayStorage()->m_sparseMap);
break;
- }
default:
break;
}
git://git.webkit.org / WebKit-https.git / commitdiff