From 921c82361145074dac639ded80b9070c7d5a830d Mon Sep 17 00:00:00 2001 From: "mhahnenberg@apple.com" Date: Wed, 16 May 2012 21:21:44 +0000 Subject: [PATCH] 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): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@117343 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Source/JavaScriptCore/ChangeLog | 34 ++++++++++++++++ .../JavaScriptCore/JavaScriptCore.def | 3 +- Source/JavaScriptCore/jit/JITStubs.cpp | 4 +- Source/JavaScriptCore/runtime/JSObject.cpp | 4 +- Source/JavaScriptCore/runtime/JSObject.h | 46 ++++++++++++++-------- Source/JavaScriptCore/runtime/Structure.cpp | 7 ++++ Source/JavaScriptCore/runtime/Structure.h | 2 + 7 files changed, 80 insertions(+), 20 deletions(-) diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index 58b2a5c..171c2c6 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,37 @@ +2012-05-16 Mark Hahnenberg + + 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 GC is not thread-safe when moving values between C stacks diff --git a/Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def b/Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def index 9c71699..4b099b2 100755 --- a/Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def +++ b/Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def @@ -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 diff --git a/Source/JavaScriptCore/jit/JITStubs.cpp b/Source/JavaScriptCore/jit/JITStubs.cpp index 73f4892..a6d6be1 100644 --- a/Source/JavaScriptCore/jit/JITStubs.cpp +++ b/Source/JavaScriptCore/jit/JITStubs.cpp @@ -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; } diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp index 8791415..baba28b 100644 --- a/Source/JavaScriptCore/runtime/JSObject.cpp +++ b/Source/JavaScriptCore/runtime/JSObject.cpp @@ -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) diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h index 7341012..0a4f1c7 100644 --- a/Source/JavaScriptCore/runtime/JSObject.h +++ b/Source/JavaScriptCore/runtime/JSObject.h @@ -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(m_propertyStorage.get()) == static_cast(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 diff --git a/Source/JavaScriptCore/runtime/Structure.cpp b/Source/JavaScriptCore/runtime/Structure.cpp index 7efe1c0..693f3f3 100644 --- a/Source/JavaScriptCore/runtime/Structure.cpp +++ b/Source/JavaScriptCore/runtime/Structure.cpp @@ -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(); diff --git a/Source/JavaScriptCore/runtime/Structure.h b/Source/JavaScriptCore/runtime/Structure.h index 31a62fd..230f59d 100644 --- a/Source/JavaScriptCore/runtime/Structure.h +++ b/Source/JavaScriptCore/runtime/Structure.h @@ -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*); -- 2.7.4