Structure::previousID() races with Structure::allocateRareData()
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Jun 2016 04:26:32 +0000 (04:26 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Jun 2016 04:26:32 +0000 (04:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=158280

Reviewed by Mark Lam.

The problem is that previousID() would test hasRareData() and then either load the
previous Structure from the rare data, or load it directly. allocateRareData() would set
the hasRareData() bit separately from moving the Structure pointer into the rare data. So
we'd have a race that would cause previousID() to sometimes return the rarae data instead
of the previous Structure.

The fix is to get rid of the hasRareData bit. We can use the structureID of the
previousOrRareData cell to determine if it's the previousID or the RareData. This fixes the
race and it's probably not any slower.

* runtime/Structure.cpp:
(JSC::Structure::Structure):
(JSC::Structure::allocateRareData):
* runtime/Structure.h:

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

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

index 3351930..f89467c 100644 (file)
@@ -1,3 +1,25 @@
+2016-06-01  Filip Pizlo  <fpizlo@apple.com>
+
+        Structure::previousID() races with Structure::allocateRareData()
+        https://bugs.webkit.org/show_bug.cgi?id=158280
+
+        Reviewed by Mark Lam.
+        
+        The problem is that previousID() would test hasRareData() and then either load the
+        previous Structure from the rare data, or load it directly. allocateRareData() would set
+        the hasRareData() bit separately from moving the Structure pointer into the rare data. So
+        we'd have a race that would cause previousID() to sometimes return the rarae data instead
+        of the previous Structure.
+
+        The fix is to get rid of the hasRareData bit. We can use the structureID of the
+        previousOrRareData cell to determine if it's the previousID or the RareData. This fixes the
+        race and it's probably not any slower.
+
+        * runtime/Structure.cpp:
+        (JSC::Structure::Structure):
+        (JSC::Structure::allocateRareData):
+        * runtime/Structure.h:
+
 2016-06-01  Michael Saboff  <msaboff@apple.com>
 
         Runaway WebContent process CPU & memory @ foxnews.com
index 6696146..4d7b53f 100644 (file)
@@ -206,7 +206,6 @@ Structure::Structure(VM& vm, JSGlobalObject* globalObject, JSValue prototype, co
     setDidPreventExtensions(false);
     setDidTransition(false);
     setStaticFunctionsReified(false);
-    setHasRareData(false);
     setTransitionWatchpointIsLikelyToBeFired(false);
     setHasBeenDictionary(false);
  
@@ -238,7 +237,6 @@ Structure::Structure(VM& vm)
     setDidPreventExtensions(false);
     setDidTransition(false);
     setStaticFunctionsReified(false);
-    setHasRareData(false);
     setTransitionWatchpointIsLikelyToBeFired(false);
     setHasBeenDictionary(false);
  
@@ -269,7 +267,6 @@ Structure::Structure(VM& vm, Structure* previous, DeferredStructureTransitionWat
     setDidPreventExtensions(previous->didPreventExtensions());
     setDidTransition(true);
     setStaticFunctionsReified(previous->staticFunctionsReified());
-    setHasRareData(false);
     setHasBeenDictionary(previous->hasBeenDictionary());
  
     TypeInfo typeInfo = previous->typeInfo();
@@ -823,11 +820,9 @@ void Structure::pin()
 void Structure::allocateRareData(VM& vm)
 {
     ASSERT(!hasRareData());
-    StructureRareData* rareData = StructureRareData::create(vm, previous());
+    StructureRareData* rareData = StructureRareData::create(vm, previousID());
     WTF::storeStoreFence();
     m_previousOrRareData.set(vm, this, rareData);
-    WTF::storeStoreFence();
-    setHasRareData(true);
     ASSERT(hasRareData());
 }
 
index 9f67ef0..21b165f 100644 (file)
@@ -274,13 +274,21 @@ public:
         
     // Will just the prototype chain intercept this property access?
     JS_EXPORT_PRIVATE bool prototypeChainMayInterceptStoreTo(VM&, PropertyName);
-        
+    
+    bool hasRareData() const
+    {
+        return isRareData(m_previousOrRareData.get());
+    }
+    
     Structure* previousID() const
     {
         ASSERT(structure()->classInfo() == info());
-        if (hasRareData())
-            return rareData()->previousID();
-        return previous();
+        // This is so written because it's used concurrently. We only load from m_previousOrRareData
+        // once, and this load is guaranteed atomic.
+        JSCell* cell = m_previousOrRareData.get();
+        if (isRareData(cell))
+            return static_cast<StructureRareData*>(cell)->previousID();
+        return static_cast<Structure*>(cell);
     }
     bool transitivelyTransitionedFrom(Structure* structureToFind);
 
@@ -602,12 +610,11 @@ public:
     DEFINE_BITFIELD(bool, didPreventExtensions, DidPreventExtensions, 1, 20);
     DEFINE_BITFIELD(bool, didTransition, DidTransition, 1, 21);
     DEFINE_BITFIELD(bool, staticFunctionsReified, StaticFunctionsReified, 1, 22);
-    DEFINE_BITFIELD(bool, hasRareData, HasRareData, 1, 23);
-    DEFINE_BITFIELD(bool, hasBeenFlattenedBefore, HasBeenFlattenedBefore, 1, 24);
-    DEFINE_BITFIELD(bool, hasCustomGetterSetterProperties, HasCustomGetterSetterProperties, 1, 25);
-    DEFINE_BITFIELD(bool, didWatchInternalProperties, DidWatchInternalProperties, 1, 26);
-    DEFINE_BITFIELD(bool, transitionWatchpointIsLikelyToBeFired, TransitionWatchpointIsLikelyToBeFired, 1, 27);
-    DEFINE_BITFIELD(bool, hasBeenDictionary, HasBeenDictionary, 1, 28);
+    DEFINE_BITFIELD(bool, hasBeenFlattenedBefore, HasBeenFlattenedBefore, 1, 23);
+    DEFINE_BITFIELD(bool, hasCustomGetterSetterProperties, HasCustomGetterSetterProperties, 1, 24);
+    DEFINE_BITFIELD(bool, didWatchInternalProperties, DidWatchInternalProperties, 1, 25);
+    DEFINE_BITFIELD(bool, transitionWatchpointIsLikelyToBeFired, TransitionWatchpointIsLikelyToBeFired, 1, 26);
+    DEFINE_BITFIELD(bool, hasBeenDictionary, HasBeenDictionary, 1, 27);
 
 private:
     friend class LLIntOffsetsExtractor;
@@ -693,11 +700,10 @@ private:
     bool isValid(ExecState*, StructureChain* cachedPrototypeChain) const;
         
     void pin();
-
-    Structure* previous() const
+    
+    bool isRareData(JSCell* cell) const
     {
-        ASSERT(!hasRareData());
-        return static_cast<Structure*>(m_previousOrRareData.get());
+        return cell && cell->structureID() != structureID();
     }
 
     StructureRareData* rareData() const