Changeset 197381 in webkit for trunk/Source/JavaScriptCore/runtime

Timestamp:

Feb 29, 2016, 7:18:59 PM (9 years ago)

Author:

[email protected]

Message:

regress/script-tests/double-pollution-putbyoffset.js.ftl-eager timed out because of a lock ordering deadlock involving InferredType and CodeBlock
​https://bugs.webkit.org/show_bug.cgi?id=154841

Reviewed by Benjamin Poulain.

Here's the deadlock:

Main thread:

1) Change an InferredType. This acquires InferredType::m_lock.
2) Fire watchpoint set. This triggers CodeBlock invalidation, which acquires

CodeBlock::m_lock.

DFG thread:

1) Iterate over the information in a CodeBlock. This acquires CodeBlock::m_lock.
2) Ask an InferredType for its descriptor(). This acquires InferredType::m_lock.

I think that the DFG thread's ordering should be legal, because the best logic for lock
hierarchies is that locks that protect the largest set of stuff should be acquired first.

This means that the main thread shouldn't be holding the InferredType::m_lock when firing
watchpoint sets. That's what this patch ensures.

At the time of writing, this test was deadlocking for me on trunk 100% of the time. With
this change I cannot get it to deadlock.

• runtime/InferredType.cpp:

(JSC::InferredType::willStoreValueSlow):
(JSC::InferredType::makeTopSlow):
(JSC::InferredType::set):
(JSC::InferredType::removeStructure):
(JSC::InferredType::InferredStructureWatchpoint::fireInternal):

• runtime/InferredType.h:

Location:

trunk/Source/JavaScriptCore/runtime

Files:

• InferredType.cpp (modified) (6 diffs)
• InferredType.h (modified) (1 diff)

trunk/Source/JavaScriptCore/runtime/InferredType.cpp

@@ -401 +401 @@
 bool InferredType::willStoreValueSlow(VM& vm, PropertyName propertyName, JSValue value)
 {
-    ConcurrentJITLocker locker(m_lock);
-    Descriptor myType = descriptor(locker);
-    Descriptor otherType = Descriptor::forValue(value);
-
-    myType.merge(otherType);
-
-    ASSERT(descriptor(locker) != myType); // The type must have changed if we're on the slow path.
-
-    set(locker, vm, propertyName, value, myType);
-
-    return kind(locker) != Top;
+    Descriptor oldType;
+    Descriptor myType;
+    bool result;
+    {
+        ConcurrentJITLocker locker(m_lock);
+        oldType = descriptor(locker);
+        myType = Descriptor::forValue(value);
+
+        myType.merge(oldType);
+        
+        ASSERT(oldType != myType); // The type must have changed if we're on the slow path.
+
+        bool setResult = set(locker, vm, myType);
+        result = kind(locker) != Top;
+        if (!setResult)
+            return result;
+    }
+    
+    InferredTypeFireDetail detail(this, propertyName.uid(), oldType, myType, value);
+    m_watchpointSet.fireAll(detail);
+    return result;
 }
 
 void InferredType::makeTopSlow(VM& vm, PropertyName propertyName)
 {
-    ConcurrentJITLocker locker(m_lock);
-    set(locker, vm, propertyName, JSValue(), Top);
-}
-
-void InferredType::set(
-    const ConcurrentJITLocker& locker, VM& vm, PropertyName propertyName, JSValue offendingValue,
-    Descriptor newDescriptor)
+    Descriptor oldType;
+    {
+        ConcurrentJITLocker locker(m_lock);
+        oldType = descriptor(locker);
+        if (!set(locker, vm, Top))
+            return;
+    }
+
+    InferredTypeFireDetail detail(this, propertyName.uid(), oldType, Top, JSValue());
+    m_watchpointSet.fireAll(detail);
+}
+
+bool InferredType::set(const ConcurrentJITLocker& locker, VM& vm, Descriptor newDescriptor)
 {
     // We will trigger write barriers while holding our lock. Currently, write barriers don't GC, but that
@@ -432 +448 @@
     // Be defensive: if we're not really changing the type, then we don't have to do anything.
     if (descriptor(locker) == newDescriptor)
-        return;
-
+        return false;
+
+    bool shouldFireWatchpointSet = false;
+    
     // The new descriptor must be more general than the previous one.
     ASSERT(newDescriptor.subsumes(descriptor(locker)));
@@ -447 +465 @@
         ASSERT(m_watchpointSet.state() != IsInvalidated);
 
-        InferredTypeFireDetail detail(
-            this, propertyName.uid(), descriptor(locker), newDescriptor, offendingValue);
-        
         // We're about to do expensive things because some compiler thread decided to watch this type and
         // then the type changed. Assume that this property is crazy, and don't ever do any more things for
@@ -455 +470 @@
         newDescriptor = Top;
 
-        m_watchpointSet.fireAll(detail);
+        shouldFireWatchpointSet = true;
     }
 
@@ -478 +493 @@
     // Assert that we did things.
     ASSERT(descriptor(locker) == newDescriptor);
+
+    return shouldFireWatchpointSet;
 }
 
@@ -487 +504 @@
     VM& vm = *Heap::heap(this)->vm();
 
-    ConcurrentJITLocker locker(m_lock);
-    
-    Descriptor newDescriptor = descriptor(locker);
-    newDescriptor.removeStructure();
-    
-    set(locker, vm, PropertyName(nullptr), JSValue(), newDescriptor);
+    Descriptor oldDescriptor;
+    Descriptor newDescriptor;
+    {
+        ConcurrentJITLocker locker(m_lock);
+        oldDescriptor = descriptor(locker);
+        newDescriptor = oldDescriptor;
+        newDescriptor.removeStructure();
+        
+        if (!set(locker, vm, newDescriptor))
+            return;
+    }
+
+    InferredTypeFireDetail detail(this, nullptr, oldDescriptor, newDescriptor, JSValue());
+    m_watchpointSet.fireAll(detail);
 }
 

trunk/Source/JavaScriptCore/runtime/InferredType.h

@@ -230 +230 @@
     bool willStoreValueSlow(VM&, PropertyName, JSValue);
     void makeTopSlow(VM&, PropertyName);
-    void set(const ConcurrentJITLocker&, VM&, PropertyName, JSValue, Descriptor);
+
+    // Helper for willStoreValueSlow() and makeTopSlow(). This returns true if we should fire the
+    // watchpoint set.
+    bool set(const ConcurrentJITLocker&, VM&, Descriptor);
+    
     void removeStructure();