regress/script-tests/double-pollution-putbyoffset.js.ftl-eager timed out because...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Mar 2016 03:18:59 +0000 (03:18 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Mar 2016 03:18:59 +0000 (03:18 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@197381 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/InferredType.cpp
Source/JavaScriptCore/runtime/InferredType.h

index 7c1392c..02b87e0 100644 (file)
@@ -1,3 +1,38 @@
+2016-02-29  Filip Pizlo  <fpizlo@apple.com>
+
+        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:
+
 2016-02-29  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [DFG][FTL][B3] Support floor and ceil
index f206533..d513522 100644 (file)
@@ -400,28 +400,44 @@ void InferredType::dump(PrintStream& out) const
 
 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.
+    Descriptor oldType;
+    Descriptor myType;
+    bool result;
+    {
+        ConcurrentJITLocker locker(m_lock);
+        oldType = descriptor(locker);
+        myType = Descriptor::forValue(value);
 
-    set(locker, vm, propertyName, value, myType);
+        myType.merge(oldType);
+        
+        ASSERT(oldType != myType); // The type must have changed if we're on the slow path.
 
-    return kind(locker) != Top;
+        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);
+    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);
 }
 
-void InferredType::set(
-    const ConcurrentJITLocker& locker, VM& vm, PropertyName propertyName, JSValue offendingValue,
-    Descriptor newDescriptor)
+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
     // could change. If it does, we don't want to deadlock. Note that we could have used
@@ -431,8 +447,10 @@ void InferredType::set(
     
     // 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)));
 
@@ -446,15 +464,12 @@ void InferredType::set(
         // We cannot have been invalidated, since if we were, then we'd already be at Top.
         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
         // it.
         newDescriptor = Top;
 
-        m_watchpointSet.fireAll(detail);
+        shouldFireWatchpointSet = true;
     }
 
     // Remove the old InferredStructure object if we no longer need it.
@@ -477,6 +492,8 @@ void InferredType::set(
 
     // Assert that we did things.
     ASSERT(descriptor(locker) == newDescriptor);
+
+    return shouldFireWatchpointSet;
 }
 
 void InferredType::removeStructure()
@@ -486,12 +503,20 @@ void InferredType::removeStructure()
     
     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);
 }
 
 void InferredType::InferredStructureWatchpoint::fireInternal(const FireDetail&)
index d60c7ba..88f41d3 100644 (file)
@@ -229,7 +229,11 @@ private:
 
     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();
 
     mutable ConcurrentJITLock m_lock;