Changeset 117343 in webkit

Timestamp:

May 16, 2012 2:21:44 PM (12 years ago)

Author:

mhahnenberg@apple.com

Message:

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

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def (modified) (3 diffs)
• jit/JITStubs.cpp (modified) (1 diff)
• runtime/JSObject.cpp (modified) (2 diffs)
• runtime/JSObject.h (modified) (7 diffs)
• runtime/Structure.cpp (modified) (1 diff)
• runtime/Structure.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def

@@ -63 +63 @@
     ?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
@@ -209 +208 @@
     ?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
@@ -320 +320 @@
     ?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

trunk/Source/JavaScriptCore/jit/JITStubs.cpp

@@ -1493 +1493 @@
     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;

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -553 +553 @@
 }
 
-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);
@@ -581 +581 @@
 
     ASSERT(newPropertyStorage);
-    m_propertyStorage.set(globalData, this, newPropertyStorage);
+    return newPropertyStorage;
 }
 

trunk/Source/JavaScriptCore/runtime/JSObject.h

@@ -213 +213 @@
         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()
@@ -453 +454 @@
 }
 
+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)
 {
@@ -664 +673 @@
             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());
@@ -679 +689 @@
     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,
@@ -728 +739 @@
         return false;
 
+    PropertyStorage newStorage = propertyStorage();
+    if (structure()->shouldGrowPropertyStorage())
+        newStorage = growPropertyStorage(globalData, structure()->propertyStorageCapacity(), structure()->suggestedNewPropertyStorageSize());
+
     Structure* structure = Structure::addPropertyTransition(globalData, this->structure(), propertyName, attributes, specificFunction, offset);
 
-    if (currentCapacity != structure->propertyStorageCapacity())
-        allocatePropertyStorage(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,
@@ -768 +780 @@
 {
     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);
 }
@@ -777 +790 @@
 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);
 }
 

trunk/Source/JavaScriptCore/runtime/Structure.cpp

@@ -267 +267 @@
 }
 
+size_t Structure::suggestedNewPropertyStorageSize()
+{
+    if (isUsingInlineStorage())
+        return JSObject::baseExternalStorageCapacity;
+    return m_propertyStorageCapacity * 2;
+}
+ 
 void Structure::despecifyDictionaryFunction(JSGlobalData& globalData, PropertyName propertyName)
 {

trunk/Source/JavaScriptCore/runtime/Structure.h

@@ -103 +103 @@
         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*);