GC in the middle of JSObject::allocatePropertyStorage can cause badness
authormhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 May 2012 21:21:44 +0000 (21:21 +0000)
committermhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 May 2012 21:21:44 +0000 (21:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=83839

Reviewed by Geoff Garen.

* JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def:
* jit/JITStubs.cpp: Making changes to use the new return value of growPropertyStorage.
(JSC::DEFINE_STUB_FUNCTION):
* runtime/JSObject.cpp:
(JSC::JSObject::growPropertyStorage): Renamed to more accurately reflect that we're
growing our already-existing PropertyStorage.
* runtime/JSObject.h:
(JSObject):
(JSC::JSObject::setPropertyStorage): "Atomically" sets the new property storage
and the new structure so that we can be sure a GC never occurs when our Structure
info is out of sync with our PropertyStorage.
(JSC):
(JSC::JSObject::putDirectInternal): Moved the check to see if we should
allocate more backing store before the actual property insertion into
the structure.
(JSC::JSObject::putDirectWithoutTransition): Ditto.
(JSC::JSObject::transitionTo): Ditto.
* runtime/Structure.cpp:
(JSC::Structure::suggestedNewPropertyStorageSize): Added to keep the resize policy
for property backing stores contained within the Structure class.
(JSC):
* runtime/Structure.h:
(JSC::Structure::shouldGrowPropertyStorage): Lets clients know if another insertion
into the Structure would require resizing the property backing store so that they can
preallocate the required storage.
(Structure):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def
Source/JavaScriptCore/jit/JITStubs.cpp
Source/JavaScriptCore/runtime/JSObject.cpp
Source/JavaScriptCore/runtime/JSObject.h
Source/JavaScriptCore/runtime/Structure.cpp
Source/JavaScriptCore/runtime/Structure.h

index 58b2a5c..171c2c6 100644 (file)
@@ -1,3 +1,37 @@
+2012-05-16  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        GC in the middle of JSObject::allocatePropertyStorage can cause badness
+        https://bugs.webkit.org/show_bug.cgi?id=83839
+
+        Reviewed by Geoff Garen.
+
+        * JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def:
+        * jit/JITStubs.cpp: Making changes to use the new return value of growPropertyStorage.
+        (JSC::DEFINE_STUB_FUNCTION):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::growPropertyStorage): Renamed to more accurately reflect that we're 
+        growing our already-existing PropertyStorage.
+        * runtime/JSObject.h:
+        (JSObject):
+        (JSC::JSObject::setPropertyStorage): "Atomically" sets the new property storage 
+        and the new structure so that we can be sure a GC never occurs when our Structure
+        info is out of sync with our PropertyStorage.
+        (JSC):
+        (JSC::JSObject::putDirectInternal): Moved the check to see if we should 
+        allocate more backing store before the actual property insertion into 
+        the structure.
+        (JSC::JSObject::putDirectWithoutTransition): Ditto.
+        (JSC::JSObject::transitionTo): Ditto.
+        * runtime/Structure.cpp:
+        (JSC::Structure::suggestedNewPropertyStorageSize): Added to keep the resize policy 
+        for property backing stores contained within the Structure class.
+        (JSC):
+        * runtime/Structure.h:
+        (JSC::Structure::shouldGrowPropertyStorage): Lets clients know if another insertion 
+        into the Structure would require resizing the property backing store so that they can 
+        preallocate the required storage.
+        (Structure):
+
 2012-05-16  Geoffrey Garen  <ggaren@apple.com>
 
         GC is not thread-safe when moving values between C stacks
index 9c71699..4b099b2 100755 (executable)
@@ -62,7 +62,6 @@ EXPORTS
     ?addSlowCase@Identifier@JSC@@CA?AV?$PassRefPtr@VStringImpl@WTF@@@WTF@@PAVExecState@2@PAVStringImpl@4@@Z
     ?addSlowCase@Identifier@JSC@@CA?AV?$PassRefPtr@VStringImpl@WTF@@@WTF@@PAVJSGlobalData@2@PAVStringImpl@4@@Z
     ?addStaticGlobals@JSGlobalObject@JSC@@IAEXPAUGlobalPropertyInfo@12@H@Z
-    ?allocatePropertyStorage@JSObject@JSC@@QAEXAAVJSGlobalData@2@II@Z
     ?allocateSlowCase@MarkedAllocator@JSC@@AAEPAXXZ
     ?append@StringBuilder@WTF@@QAEXPBEI@Z
     ?append@StringBuilder@WTF@@QAEXPB_WI@Z
@@ -208,6 +207,7 @@ EXPORTS
     ?globalExec@JSGlobalObject@JSC@@QAEPAVExecState@2@XZ
     ?globalObjectCount@Heap@JSC@@QAEIXZ
     ?grow@HandleSet@JSC@@AAEXXZ
+    ?growPropertyStorage@JSObject@JSC@@QAEPAV?$WriteBarrierBase@W4Unknown@JSC@@@2@AAVJSGlobalData@2@II@Z
     ?hasInstance@JSObject@JSC@@SA_NPAV12@PAVExecState@2@VJSValue@2@2@Z
     ?hasProperty@JSObject@JSC@@QBE_NPAVExecState@2@I@Z
     ?hasProperty@JSObject@JSC@@QBE_NPAVExecState@2@VPropertyName@2@@Z
@@ -319,6 +319,7 @@ EXPORTS
     ?stopProfiling@Profiler@JSC@@QAE?AV?$PassRefPtr@VProfile@JSC@@@WTF@@PAVExecState@2@ABVUString@2@@Z
     ?stopSampling@JSGlobalData@JSC@@QAEXXZ
     ?substringSharingImpl@UString@JSC@@QBE?AV12@II@Z
+    ?suggestedNewPropertyStorageSize@Structure@JSC@@QAEIXZ
     ?synthesizePrototype@JSValue@JSC@@QBEPAVJSObject@2@PAVExecState@2@@Z
     ?thisObject@DebuggerCallFrame@JSC@@QBEPAVJSObject@2@XZ
     ?throwError@JSC@@YA?AVJSValue@1@PAVExecState@1@V21@@Z
index 73f4892..a6d6be1 100644 (file)
@@ -1492,7 +1492,9 @@ DEFINE_STUB_FUNCTION(JSObject*, op_put_by_id_transition_realloc)
 
     ASSERT(baseValue.isObject());
     JSObject* base = asObject(baseValue);
-    base->allocatePropertyStorage(*stackFrame.globalData, oldSize, newSize);
+    JSGlobalData& globalData = *stackFrame.globalData;
+    PropertyStorage newStorage = base->growPropertyStorage(globalData, oldSize, newSize);
+    base->setPropertyStorage(globalData, newStorage, newStructure);
 
     return base;
 }
index 8791415..baba28b 100644 (file)
@@ -552,7 +552,7 @@ Structure* JSObject::createInheritorID(JSGlobalData& globalData)
     return m_inheritorID.get();
 }
 
-void JSObject::allocatePropertyStorage(JSGlobalData& globalData, size_t oldSize, size_t newSize)
+PropertyStorage JSObject::growPropertyStorage(JSGlobalData& globalData, size_t oldSize, size_t newSize)
 {
     ASSERT(newSize > oldSize);
 
@@ -580,7 +580,7 @@ void JSObject::allocatePropertyStorage(JSGlobalData& globalData, size_t oldSize,
     }
 
     ASSERT(newPropertyStorage);
-    m_propertyStorage.set(globalData, this, newPropertyStorage);
+    return newPropertyStorage;
 }
 
 bool JSObject::getOwnPropertyDescriptor(JSObject* object, ExecState* exec, PropertyName propertyName, PropertyDescriptor& descriptor)
index 7341012..0a4f1c7 100644 (file)
@@ -212,8 +212,9 @@ namespace JSC {
         bool staticFunctionsReified() { return structure()->staticFunctionsReified(); }
         void reifyStaticFunctionsForDelete(ExecState* exec);
 
-        JS_EXPORT_PRIVATE void allocatePropertyStorage(JSGlobalData&, size_t oldSize, size_t newSize);
+        JS_EXPORT_PRIVATE PropertyStorage growPropertyStorage(JSGlobalData&, size_t oldSize, size_t newSize);
         bool isUsingInlineStorage() const { return static_cast<const void*>(m_propertyStorage.get()) == static_cast<const void*>(this + 1); }
+        void setPropertyStorage(JSGlobalData&, PropertyStorage, Structure*);
 
         void* addressOfPropertyStorage()
         {
@@ -452,6 +453,14 @@ inline bool JSObject::isGlobalThis() const
     return structure()->typeInfo().type() == GlobalThisType;
 }
 
+inline void JSObject::setPropertyStorage(JSGlobalData& globalData, PropertyStorage storage, Structure* structure)
+{
+    ASSERT(storage);
+    ASSERT(structure);
+    setStructure(globalData, structure);
+    m_propertyStorage.set(globalData, this, storage);
+}
+
 inline JSObject* constructEmptyObject(ExecState* exec, Structure* structure)
 {
     return JSFinalObject::create(exec, structure);
@@ -663,10 +672,11 @@ inline bool JSObject::putDirectInternal(JSGlobalData& globalData, PropertyName p
         if ((mode == PutModePut) && !isExtensible())
             return false;
 
-        size_t currentCapacity = structure()->propertyStorageCapacity();
+        PropertyStorage newStorage = propertyStorage();
+        if (structure()->shouldGrowPropertyStorage())
+            newStorage = growPropertyStorage(globalData, structure()->propertyStorageCapacity(), structure()->suggestedNewPropertyStorageSize());
         offset = structure()->addPropertyWithoutTransition(globalData, propertyName, attributes, specificFunction);
-        if (currentCapacity != structure()->propertyStorageCapacity())
-            allocatePropertyStorage(globalData, currentCapacity, structure()->propertyStorageCapacity());
+        setPropertyStorage(globalData, newStorage, structure());
 
         ASSERT(offset < structure()->propertyStorageCapacity());
         putDirectOffset(globalData, offset, value);
@@ -678,12 +688,13 @@ inline bool JSObject::putDirectInternal(JSGlobalData& globalData, PropertyName p
 
     size_t offset;
     size_t currentCapacity = structure()->propertyStorageCapacity();
-    if (Structure* structure = Structure::addPropertyTransitionToExistingStructure(this->structure(), propertyName, attributes, specificFunction, offset)) {    
+    if (Structure* structure = Structure::addPropertyTransitionToExistingStructure(this->structure(), propertyName, attributes, specificFunction, offset)) {
+        PropertyStorage newStorage = propertyStorage(); 
         if (currentCapacity != structure->propertyStorageCapacity())
-            allocatePropertyStorage(globalData, currentCapacity, structure->propertyStorageCapacity());
+            newStorage = growPropertyStorage(globalData, currentCapacity, structure->propertyStorageCapacity());
 
         ASSERT(offset < structure->propertyStorageCapacity());
-        setStructure(globalData, structure);
+        setPropertyStorage(globalData, newStorage, structure);
         putDirectOffset(globalData, offset, value);
         // This is a new property; transitions with specific values are not currently cachable,
         // so leave the slot in an uncachable state.
@@ -727,13 +738,14 @@ inline bool JSObject::putDirectInternal(JSGlobalData& globalData, PropertyName p
     if ((mode == PutModePut) && !isExtensible())
         return false;
 
-    Structure* structure = Structure::addPropertyTransition(globalData, this->structure(), propertyName, attributes, specificFunction, offset);
+    PropertyStorage newStorage = propertyStorage();
+    if (structure()->shouldGrowPropertyStorage())
+        newStorage = growPropertyStorage(globalData, structure()->propertyStorageCapacity(), structure()->suggestedNewPropertyStorageSize());
 
-    if (currentCapacity != structure->propertyStorageCapacity())
-        allocatePropertyStorage(globalData, currentCapacity, structure->propertyStorageCapacity());
+    Structure* structure = Structure::addPropertyTransition(globalData, this->structure(), propertyName, attributes, specificFunction, offset);
 
     ASSERT(offset < structure->propertyStorageCapacity());
-    setStructure(globalData, structure);
+    setPropertyStorage(globalData, newStorage, structure);
     putDirectOffset(globalData, offset, value);
     // This is a new property; transitions with specific values are not currently cachable,
     // so leave the slot in an uncachable state.
@@ -767,18 +779,20 @@ inline void JSObject::putDirect(JSGlobalData& globalData, PropertyName propertyN
 inline void JSObject::putDirectWithoutTransition(JSGlobalData& globalData, PropertyName propertyName, JSValue value, unsigned attributes)
 {
     ASSERT(!value.isGetterSetter() && !(attributes & Accessor));
-    size_t currentCapacity = structure()->propertyStorageCapacity();
+    PropertyStorage newStorage = propertyStorage();
+    if (structure()->shouldGrowPropertyStorage())
+        newStorage = growPropertyStorage(globalData, structure()->propertyStorageCapacity(), structure()->suggestedNewPropertyStorageSize());
     size_t offset = structure()->addPropertyWithoutTransition(globalData, propertyName, attributes, getJSFunction(value));
-    if (currentCapacity != structure()->propertyStorageCapacity())
-        allocatePropertyStorage(globalData, currentCapacity, structure()->propertyStorageCapacity());
+    setPropertyStorage(globalData, newStorage, structure());
     putDirectOffset(globalData, offset, value);
 }
 
 inline void JSObject::transitionTo(JSGlobalData& globalData, Structure* newStructure)
 {
+    PropertyStorage newStorage = propertyStorage();
     if (structure()->propertyStorageCapacity() != newStructure->propertyStorageCapacity())
-        allocatePropertyStorage(globalData, structure()->propertyStorageCapacity(), newStructure->propertyStorageCapacity());
-    setStructure(globalData, newStructure);
+        newStorage = growPropertyStorage(globalData, structure()->propertyStorageCapacity(), newStructure->propertyStorageCapacity());
+    setPropertyStorage(globalData, newStorage, newStructure);
 }
 
 inline JSValue JSObject::toPrimitive(ExecState* exec, PreferredPrimitiveType preferredType) const
index 7efe1c0..693f3f3 100644 (file)
@@ -266,6 +266,13 @@ void Structure::growPropertyStorageCapacity()
         m_propertyStorageCapacity *= 2;
 }
 
+size_t Structure::suggestedNewPropertyStorageSize()
+{
+    if (isUsingInlineStorage())
+        return JSObject::baseExternalStorageCapacity;
+    return m_propertyStorageCapacity * 2;
+}
 void Structure::despecifyDictionaryFunction(JSGlobalData& globalData, PropertyName propertyName)
 {
     StringImpl* rep = propertyName.impl();
index 31a62fd..230f59d 100644 (file)
@@ -102,6 +102,8 @@ namespace JSC {
         bool isFrozen(JSGlobalData&);
         bool isExtensible() const { return !m_preventExtensions; }
         bool didTransition() const { return m_didTransition; }
+        bool shouldGrowPropertyStorage() { return propertyStorageCapacity() == propertyStorageSize(); }
+        JS_EXPORT_PRIVATE size_t suggestedNewPropertyStorageSize(); 
 
         Structure* flattenDictionaryStructure(JSGlobalData&, JSObject*);