Diff
Modified: branches/safari-603-branch/Source/_javascript_Core/ChangeLog (210658 => 210659)
--- branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-12 16:46:02 UTC (rev 210658)
+++ branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-12 16:46:05 UTC (rev 210659)
@@ -1,5 +1,41 @@
2017-01-12 Matthew Hanson <matthew_han...@apple.com>
+ Merge r210553. rdar://problem/29941356
+
+ 2017-01-09 Filip Pizlo <fpi...@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-12 Matthew Hanson <matthew_han...@apple.com>
+
Merge r210530. rdar://problem/29909896
2017-01-09 Filip Pizlo <fpi...@apple.com>
Modified: branches/safari-603-branch/Source/_javascript_Core/heap/DeferGC.h (210658 => 210659)
--- branches/safari-603-branch/Source/_javascript_Core/heap/DeferGC.h 2017-01-12 16:46:02 UTC (rev 210658)
+++ branches/safari-603-branch/Source/_javascript_Core/heap/DeferGC.h 2017-01-12 16:46:05 UTC (rev 210659)
@@ -1,5 +1,5 @@
/*
- * 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
@@ -67,31 +67,41 @@
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
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/JSArray.cpp (210658 => 210659)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/JSArray.cpp 2017-01-12 16:46:02 UTC (rev 210658)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/JSArray.cpp 2017-01-12 16:46:05 UTC (rev 210659)
@@ -1,6 +1,6 @@
/*
* Copyright (C) 1999-2000 Harri Porten (por...@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 (p...@post.com)
* Copyright (C) 2006 Alexey Proskuryakov (a...@nypop.com)
*
@@ -298,7 +298,7 @@
}
// 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();
@@ -892,6 +892,9 @@
if (startIndex >= vectorLength)
return true;
+ DisallowGC disallowGC;
+ auto locker = holdLock(*this);
+
if (startIndex + count > vectorLength)
count = vectorLength - startIndex;
@@ -930,7 +933,7 @@
// 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;
@@ -1098,7 +1101,7 @@
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);
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/JSArray.h (210658 => 210659)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/JSArray.h 2017-01-12 16:46:02 UTC (rev 210658)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/JSArray.h 2017-01-12 16:46:05 UTC (rev 210659)
@@ -1,6 +1,6 @@
/*
* Copyright (C) 1999-2000 Harri Porten (por...@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
@@ -177,7 +177,7 @@
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);
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/JSObject.cpp (210658 => 210659)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/JSObject.cpp 2017-01-12 16:46:02 UTC (rev 210658)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/JSObject.cpp 2017-01-12 16:46:05 UTC (rev 210659)
@@ -383,6 +383,20 @@
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)
@@ -395,27 +409,17 @@
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;
}
