Changeset 210947 in webkit for trunk/Source/JavaScriptCore/runtime/StructureInlines.h

Timestamp:

Jan 19, 2017, 6:38:45 PM (8 years ago)

Author:

[email protected]

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):

File:

• trunk/Source/JavaScriptCore/runtime/StructureInlines.h (modified) (6 diffs)

trunk/Source/JavaScriptCore/runtime/StructureInlines.h

@@ -244 +244 @@
 }
 
-ALWAYS_INLINE bool Structure::checkOffsetConsistency() const
-{
-    PropertyTable* propertyTable = propertyTableOrNull();
-
-    if (!propertyTable) {
-        ASSERT(!isPinnedPropertyTable());
-        return true;
-    }
-
+template<typename DetailsFunc>
+ALWAYS_INLINE bool Structure::checkOffsetConsistency(PropertyTable* propertyTable, const DetailsFunc& detailsFunc) const
+{
     // 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
@@ -273 +267 @@
         dataLog("inlineOverflowAccordingToTotalSize = ", inlineOverflowAccordingToTotalSize, "\n");
         dataLog("numberOfOutOfLineSlotsForLastOffset = ", numberOfOutOfLineSlotsForLastOffset(m_offset), "\n");
+        detailsFunc();
         UNREACHABLE_FOR_PLATFORM();
     };
@@ -284 +279 @@
 }
 
+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()
 {
@@ -303 +317 @@
 }
 
-template<typename Func>
+template<Structure::ShouldPin shouldPin, typename Func>
 inline PropertyOffset Structure::add(VM& vm, PropertyName propertyName, unsigned attributes, const Func& func)
 {
@@ -309 +323 @@
 
     GCSafeConcurrentJSLocker locker(m_lock, vm.heap);
-    
-    setPropertyTable(vm, table);
+
+    switch (shouldPin) {
+    case ShouldPin::Yes:
+        pin(locker, vm, table);
+        break;
+    case ShouldPin::No:
+        setPropertyTable(vm, table);
+        break;
+    }
     
     ASSERT(!JSC::isValidOffset(get(vm, propertyName)));
@@ -367 +388 @@
 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);
 }