Diff
Modified: branches/safari-603-branch/Source/_javascript_Core/ChangeLog (210967 => 210968)
--- branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-20 18:09:13 UTC (rev 210967)
+++ branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-20 18:09:17 UTC (rev 210968)
@@ -1,5 +1,50 @@
2017-01-20 Matthew Hanson <[email protected]>
+ Merge r210947. rdar://problem/30108809
+
+ 2017-01-19 Filip Pizlo <[email protected]>
+
+ Structure::pin() needs to be called while holding a lock
+ https://bugs.webkit.org/show_bug.cgi?id=167220
+
+ Reviewed by Saam Barati.
+
+ Imagine this race: the mutator calls pin() and the collector calls visitChildren(),
+ on the same Structure at the same time. In trunk pin() does not require a lock to be
+ held and it doesn't grab any locks. Meanwhile visitChildren() grabs the lock, checks
+ if the structure is pinned, and if not, it removes it by overwriting with zero. Now
+ imagine how this plays out when pin() runs. Since pin() grabs no locks, it is
+ irrelevant that visitChildren() grabs any locks. So, visitChildren() might check if
+ the table is pinned before pin() pins it, and then clear the table after it was
+ already pinned.
+
+ The problem here is that pin() should be holding a lock. We could either make pin()
+ grab that lock by itself, or what this patch does is makes the caller grab the lock.
+ This is great because it means that sometimes we don't have to introduce any new
+ locking.
+
+ This fixes a materializePropertyTable() checkOffsetConsistency() crash that happens
+ very rarely, but I was able to get it to reproduce with run-webkit-tests and
+ aggressive GC settings.
+
+ * runtime/ConcurrentJSLock.h:
+ * runtime/Structure.cpp:
+ (JSC::Structure::materializePropertyTable):
+ (JSC::Structure::changePrototypeTransition):
+ (JSC::Structure::attributeChangeTransition):
+ (JSC::Structure::toDictionaryTransition):
+ (JSC::Structure::nonPropertyTransition):
+ (JSC::Structure::pin):
+ (JSC::Structure::pinForCaching):
+ (JSC::Structure::add):
+ * runtime/Structure.h:
+ * runtime/StructureInlines.h:
+ (JSC::Structure::checkOffsetConsistency):
+ (JSC::Structure::add):
+ (JSC::Structure::addPropertyWithoutTransition):
+
+2017-01-20 Matthew Hanson <[email protected]>
+
Merge r210935. rdar://problem/30101860
2017-01-19 Filip Pizlo <[email protected]>
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/ConcurrentJSLock.h (210967 => 210968)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/ConcurrentJSLock.h 2017-01-20 18:09:13 UTC (rev 210967)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/ConcurrentJSLock.h 2017-01-20 18:09:17 UTC (rev 210968)
@@ -40,7 +40,7 @@
typedef NoLockLocker ConcurrentJSLockerImpl;
#endif
-class ConcurrentJSLockerBase {
+class ConcurrentJSLockerBase : public AbstractLocker {
WTF_MAKE_NONCOPYABLE(ConcurrentJSLockerBase);
public:
explicit ConcurrentJSLockerBase(ConcurrentJSLock& lockable)
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.cpp (210967 => 210968)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.cpp 2017-01-20 18:09:13 UTC (rev 210967)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.cpp 2017-01-20 18:09:17 UTC (rev 210968)
@@ -357,7 +357,17 @@
table->add(entry, m_offset, PropertyTable::PropertyOffsetMustNotChange);
}
- checkOffsetConsistency();
+ checkOffsetConsistency(
+ table,
+ [&] () {
+ dataLog("Detected in materializePropertyTable.\n");
+ dataLog("Found structure = ", RawPointer(structure), "\n");
+ dataLog("structures = ");
+ CommaPrinter comma;
+ for (Structure* structure : structures)
+ dataLog(comma, RawPointer(structure));
+ dataLog("\n");
+ });
return table;
}
@@ -546,8 +556,9 @@
Structure* transition = create(vm, structure);
transition->m_prototype.set(vm, transition, prototype);
-
- transition->pin(vm, structure->copyPropertyTableForPinning(vm));
+
+ PropertyTable* table = structure->copyPropertyTableForPinning(vm);
+ transition->pin(holdLock(transition->m_lock), vm, table);
transition->m_offset = structure->m_offset;
transition->checkOffsetConsistency();
@@ -558,8 +569,9 @@
{
if (!structure->isUncacheableDictionary()) {
Structure* transition = create(vm, structure);
-
- transition->pin(vm, structure->copyPropertyTableForPinning(vm));
+
+ PropertyTable* table = structure->copyPropertyTableForPinning(vm);
+ transition->pin(holdLock(transition->m_lock), vm, table);
transition->m_offset = structure->m_offset;
structure = transition;
@@ -580,7 +592,8 @@
Structure* transition = create(vm, structure, deferred);
- transition->pin(vm, structure->copyPropertyTableForPinning(vm));
+ PropertyTable* table = structure->copyPropertyTableForPinning(vm);
+ transition->pin(holdLock(transition->m_lock), vm, table);
transition->m_offset = structure->m_offset;
transition->setDictionaryKind(kind);
transition->setHasBeenDictionary(true);
@@ -667,11 +680,12 @@
// We pin the property table on transitions that do wholesale editing of the property
// table, since our logic for walking the property transition chain to rematerialize the
// table doesn't know how to take into account such wholesale edits.
-
- transition->pinForCaching(vm, structure->copyPropertyTableForPinning(vm));
+
+ PropertyTable* table = structure->copyPropertyTableForPinning(vm);
+ transition->pinForCaching(holdLock(transition->m_lock), vm, table);
transition->m_offset = structure->m_offset;
- PropertyTable* table = transition->propertyTableOrNull();
+ table = transition->propertyTableOrNull();
RELEASE_ASSERT(table);
for (auto& entry : *table) {
if (setsDontDeleteOnAllProperties(transitionKind))
@@ -689,10 +703,11 @@
&& !transition->propertyTableOrNull()->isEmpty())
transition->setHasReadOnlyOrGetterSetterPropertiesExcludingProto(true);
- if (structure->isDictionary())
- transition->pin(vm, transition->ensurePropertyTable(vm));
- else {
- ConcurrentJSLocker locker(structure->m_lock);
+ if (structure->isDictionary()) {
+ PropertyTable* table = transition->ensurePropertyTable(vm);
+ transition->pin(holdLock(transition->m_lock), vm, table);
+ } else {
+ auto locker = holdLock(structure->m_lock);
structure->m_transitionTable.add(vm, transition);
}
@@ -804,7 +819,7 @@
return this;
}
-void Structure::pin(VM& vm, PropertyTable* table)
+void Structure::pin(const AbstractLocker&, VM& vm, PropertyTable* table)
{
setIsPinnedPropertyTable(true);
setPropertyTable(vm, table);
@@ -812,7 +827,7 @@
m_nameInPrevious = nullptr;
}
-void Structure::pinForCaching(VM& vm, PropertyTable* table)
+void Structure::pinForCaching(const AbstractLocker&, VM& vm, PropertyTable* table)
{
setIsPinnedPropertyTable(true);
setPropertyTable(vm, table);
@@ -978,7 +993,7 @@
PropertyOffset Structure::add(VM& vm, PropertyName propertyName, unsigned attributes)
{
- return add(
+ return add<ShouldPin::No>(
vm, propertyName, attributes,
[this] (const GCSafeConcurrentJSLocker&, PropertyOffset, PropertyOffset newLastOffset) {
setLastOffset(newLastOffset);
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.h (210967 => 210968)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.h 2017-01-20 18:09:13 UTC (rev 210967)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.h 2017-01-20 18:09:17 UTC (rev 210968)
@@ -660,8 +660,9 @@
void findStructuresAndMapForMaterialization(Vector<Structure*, 8>& structures, Structure*&, PropertyTable*&);
static Structure* toDictionaryTransition(VM&, Structure*, DictionaryKind, DeferredStructureTransitionWatchpointFire* = nullptr);
-
- template<typename Func>
+
+ enum class ShouldPin { No, Yes };
+ template<ShouldPin, typename Func>
PropertyOffset add(VM&, PropertyName, unsigned attributes, const Func&);
PropertyOffset add(VM&, PropertyName, unsigned attributes);
template<typename Func>
@@ -725,9 +726,10 @@
bool isValid(JSGlobalObject*, StructureChain* cachedPrototypeChain) const;
bool isValid(ExecState*, StructureChain* cachedPrototypeChain) const;
-
- JS_EXPORT_PRIVATE void pin(VM&, PropertyTable*);
- void pinForCaching(VM&, PropertyTable*);
+
+ // You have to hold the structure lock to do these.
+ JS_EXPORT_PRIVATE void pin(const AbstractLocker&, VM&, PropertyTable*);
+ void pinForCaching(const AbstractLocker&, VM&, PropertyTable*);
bool isRareData(JSCell* cell) const
{
@@ -740,6 +742,8 @@
return static_cast<StructureRareData*>(m_previousOrRareData.get());
}
+ template<typename DetailsFunc>
+ bool checkOffsetConsistency(PropertyTable*, const DetailsFunc&) const;
bool checkOffsetConsistency() const;
JS_EXPORT_PRIVATE void allocateRareData(VM&);
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/StructureInlines.h (210967 => 210968)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/StructureInlines.h 2017-01-20 18:09:13 UTC (rev 210967)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/StructureInlines.h 2017-01-20 18:09:17 UTC (rev 210968)
@@ -243,15 +243,9 @@
return map->get(offset);
}
-ALWAYS_INLINE bool Structure::checkOffsetConsistency() const
+template<typename DetailsFunc>
+ALWAYS_INLINE bool Structure::checkOffsetConsistency(PropertyTable* propertyTable, const DetailsFunc& detailsFunc) const
{
- PropertyTable* propertyTable = propertyTableOrNull();
-
- if (!propertyTable) {
- ASSERT(!isPinnedPropertyTable());
- return true;
- }
-
// We cannot reliably assert things about the property table in the concurrent
// compilation thread. It is possible for the table to be stolen and then have
// things added to it, which leads to the offsets being all messed up. We could
@@ -272,6 +266,7 @@
dataLog("totalSize = ", totalSize, "\n");
dataLog("inlineOverflowAccordingToTotalSize = ", inlineOverflowAccordingToTotalSize, "\n");
dataLog("numberOfOutOfLineSlotsForLastOffset = ", numberOfOutOfLineSlotsForLastOffset(m_offset), "\n");
+ detailsFunc();
UNREACHABLE_FOR_PLATFORM();
};
@@ -283,6 +278,25 @@
return true;
}
+ALWAYS_INLINE bool Structure::checkOffsetConsistency() const
+{
+ PropertyTable* propertyTable = propertyTableOrNull();
+
+ if (!propertyTable) {
+ ASSERT(!isPinnedPropertyTable());
+ return true;
+ }
+
+ // We cannot reliably assert things about the property table in the concurrent
+ // compilation thread. It is possible for the table to be stolen and then have
+ // things added to it, which leads to the offsets being all messed up. We could
+ // get around this by grabbing a lock here, but I think that would be overkill.
+ if (isCompilationThread())
+ return true;
+
+ return checkOffsetConsistency(propertyTable, [] () { });
+}
+
inline void Structure::checkConsistency()
{
checkOffsetConsistency();
@@ -302,15 +316,22 @@
rareData()->setObjectToStringValue(exec, vm, this, value, toStringTagSymbolSlot);
}
-template<typename Func>
+template<Structure::ShouldPin shouldPin, typename Func>
inline PropertyOffset Structure::add(VM& vm, PropertyName propertyName, unsigned attributes, const Func& func)
{
PropertyTable* table = ensurePropertyTable(vm);
GCSafeConcurrentJSLocker locker(m_lock, vm.heap);
+
+ switch (shouldPin) {
+ case ShouldPin::Yes:
+ pin(locker, vm, table);
+ break;
+ case ShouldPin::No:
+ setPropertyTable(vm, table);
+ break;
+ }
- setPropertyTable(vm, table);
-
ASSERT(!JSC::isValidOffset(get(vm, propertyName)));
checkConsistency();
@@ -366,9 +387,7 @@
template<typename Func>
inline PropertyOffset Structure::addPropertyWithoutTransition(VM& vm, PropertyName propertyName, unsigned attributes, const Func& func)
{
- pin(vm, ensurePropertyTable(vm));
-
- return add(vm, propertyName, attributes, func);
+ return add<ShouldPin::Yes>(vm, propertyName, attributes, func);
}
template<typename Func>
