- Revision
- 210947
- Author
- [email protected]
- Date
- 2017-01-19 18:38:45 -0800 (Thu, 19 Jan 2017)
Log Message
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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (210946 => 210947)
--- trunk/Source/_javascript_Core/ChangeLog 2017-01-20 01:35:58 UTC (rev 210946)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-01-20 02:38:45 UTC (rev 210947)
@@ -1,5 +1,46 @@
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-19 Filip Pizlo <[email protected]>
+
The mutator needs to fire a barrier after memmoving stuff around in an object that the GC scans
https://bugs.webkit.org/show_bug.cgi?id=167208
Modified: trunk/Source/_javascript_Core/runtime/ConcurrentJSLock.h (210946 => 210947)
--- trunk/Source/_javascript_Core/runtime/ConcurrentJSLock.h 2017-01-20 01:35:58 UTC (rev 210946)
+++ trunk/Source/_javascript_Core/runtime/ConcurrentJSLock.h 2017-01-20 02:38:45 UTC (rev 210947)
@@ -40,7 +40,7 @@
typedef NoLockLocker ConcurrentJSLockerImpl;
#endif
-class ConcurrentJSLockerBase {
+class ConcurrentJSLockerBase : public AbstractLocker {
WTF_MAKE_NONCOPYABLE(ConcurrentJSLockerBase);
public:
explicit ConcurrentJSLockerBase(ConcurrentJSLock& lockable)
Modified: trunk/Source/_javascript_Core/runtime/Structure.cpp (210946 => 210947)
--- trunk/Source/_javascript_Core/runtime/Structure.cpp 2017-01-20 01:35:58 UTC (rev 210946)
+++ trunk/Source/_javascript_Core/runtime/Structure.cpp 2017-01-20 02:38:45 UTC (rev 210947)
@@ -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: trunk/Source/_javascript_Core/runtime/Structure.h (210946 => 210947)
--- trunk/Source/_javascript_Core/runtime/Structure.h 2017-01-20 01:35:58 UTC (rev 210946)
+++ trunk/Source/_javascript_Core/runtime/Structure.h 2017-01-20 02:38:45 UTC (rev 210947)
@@ -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: trunk/Source/_javascript_Core/runtime/StructureInlines.h (210946 => 210947)
--- trunk/Source/_javascript_Core/runtime/StructureInlines.h 2017-01-20 01:35:58 UTC (rev 210946)
+++ trunk/Source/_javascript_Core/runtime/StructureInlines.h 2017-01-20 02:38:45 UTC (rev 210947)
@@ -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>
