[JSC] Potential GC fix for JSPropertyNameEnumerator
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 27 Jul 2019 04:55:11 +0000 (04:55 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 27 Jul 2019 04:55:11 +0000 (04:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200151

Reviewed by Mark Lam.

JSTests:

* stress/for-in-stress.js: Added.
(keys):

Source/JavaScriptCore:

We have been seeing some JSPropertyNameEnumerator::visitChildren crashes for a long time. The crash frequency itself is not high, but it has existed for a long time.
The crash happens when visiting m_propertyNames. It is also possible that this crash is caused by random corruption somewhere, but JSPropertyNameEnumerator
has some tricky (and potentially dangerous) implementations anyway.

1. JSPropertyNameEnumerator have Vector<WriteBarrier<JSString>> and it is extended in finishCreation with a lock.
   We should use Auxiliary memory for this use case. And we should set this memory in the constructor so that
   we do not extend it in finishCreation, and we do not need a lock.
2. JSPropertyNameEnumerator gets StructureID before allocating JSPropertyNameEnumerator. This is potentially dangerous because the conservative scan
   cannot find the Structure* since we could only have StructureID. Since allocation code happens after StructureID is retrieved, it is possible that
   the allocation causes GC and Structure* is collected.

In this patch, we align JSPropertyNameEnumerator implementation to the modern one to avoid using Vector<WriteBarrier<JSString>>. And we can make JSPropertyNameEnumerator
a non-destructible cell. Since JSCell's destructor is one of the cause of various issues, we should avoid it if we can.

No behavior change. This patch adds a test stressing JSPropertyNameEnumerator.

* dfg/DFGOperations.cpp:
* runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL):
* runtime/JSPropertyNameEnumerator.cpp:
(JSC::JSPropertyNameEnumerator::create):
(JSC::JSPropertyNameEnumerator::JSPropertyNameEnumerator):
(JSC::JSPropertyNameEnumerator::finishCreation):
(JSC::JSPropertyNameEnumerator::visitChildren):
(JSC::JSPropertyNameEnumerator::destroy): Deleted.
* runtime/JSPropertyNameEnumerator.h:
* runtime/VM.cpp:
(JSC::VM::emptyPropertyNameEnumeratorSlow):
* runtime/VM.h:
(JSC::VM::emptyPropertyNameEnumerator):

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

JSTests/ChangeLog
JSTests/stress/for-in-stress.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp
Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h
Source/JavaScriptCore/runtime/VM.cpp
Source/JavaScriptCore/runtime/VM.h

index 04d011a..c518b77 100644 (file)
@@ -1,3 +1,13 @@
+2019-07-26  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Potential GC fix for JSPropertyNameEnumerator
+        https://bugs.webkit.org/show_bug.cgi?id=200151
+
+        Reviewed by Mark Lam.
+
+        * stress/for-in-stress.js: Added.
+        (keys):
+
 2019-07-25  Ross Kirsling  <ross.kirsling@sony.com>
 
         Legacy numeric literals should not permit separators or BigInt
diff --git a/JSTests/stress/for-in-stress.js b/JSTests/stress/for-in-stress.js
new file mode 100644 (file)
index 0000000..67cd405
--- /dev/null
@@ -0,0 +1,29 @@
+var counter = 0;
+function keys(a, b, c) {
+    for (var i in a) {
+        for (var j in b) {
+            for (var k in c) {
+            }
+        }
+    }
+    if ((++counter) % 1000 === 0)
+        gc();
+}
+noInline(keys);
+
+var dictionary = {
+    0: 2,
+    "Hey": "Hello",
+    "World": 32.4,
+    "deleted": 20,
+};
+delete dictionary["deleted"];
+for (var i = 0; i < 1e4; ++i) {
+    keys([], [20], ["Hey"]);
+    keys(["OK", 30], { Hello: 0, World: 2 }, []);
+    keys(["OK", 30], { Hello: 0, World: 2 }, [42]);
+    keys(["OK", 30], [], { Hello: 0, World: 2 });
+    keys(["OK", 30], [2.5, 3.7], dictionary);
+    keys(dictionary, { Hello: 0, World: 2 }, dictionary);
+    keys({ Hello: 0, World: 2 }, dictionary, { Hello: 0, World: 2, 3:42 });
+}
index 0b40ce4..23b5d8a 100644 (file)
@@ -1,3 +1,41 @@
+2019-07-26  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Potential GC fix for JSPropertyNameEnumerator
+        https://bugs.webkit.org/show_bug.cgi?id=200151
+
+        Reviewed by Mark Lam.
+
+        We have been seeing some JSPropertyNameEnumerator::visitChildren crashes for a long time. The crash frequency itself is not high, but it has existed for a long time.
+        The crash happens when visiting m_propertyNames. It is also possible that this crash is caused by random corruption somewhere, but JSPropertyNameEnumerator
+        has some tricky (and potentially dangerous) implementations anyway.
+
+        1. JSPropertyNameEnumerator have Vector<WriteBarrier<JSString>> and it is extended in finishCreation with a lock.
+           We should use Auxiliary memory for this use case. And we should set this memory in the constructor so that
+           we do not extend it in finishCreation, and we do not need a lock.
+        2. JSPropertyNameEnumerator gets StructureID before allocating JSPropertyNameEnumerator. This is potentially dangerous because the conservative scan
+           cannot find the Structure* since we could only have StructureID. Since allocation code happens after StructureID is retrieved, it is possible that
+           the allocation causes GC and Structure* is collected.
+
+        In this patch, we align JSPropertyNameEnumerator implementation to the modern one to avoid using Vector<WriteBarrier<JSString>>. And we can make JSPropertyNameEnumerator
+        a non-destructible cell. Since JSCell's destructor is one of the cause of various issues, we should avoid it if we can.
+
+        No behavior change. This patch adds a test stressing JSPropertyNameEnumerator.
+
+        * dfg/DFGOperations.cpp:
+        * runtime/CommonSlowPaths.cpp:
+        (JSC::SLOW_PATH_DECL):
+        * runtime/JSPropertyNameEnumerator.cpp:
+        (JSC::JSPropertyNameEnumerator::create):
+        (JSC::JSPropertyNameEnumerator::JSPropertyNameEnumerator):
+        (JSC::JSPropertyNameEnumerator::finishCreation):
+        (JSC::JSPropertyNameEnumerator::visitChildren):
+        (JSC::JSPropertyNameEnumerator::destroy): Deleted.
+        * runtime/JSPropertyNameEnumerator.h:
+        * runtime/VM.cpp:
+        (JSC::VM::emptyPropertyNameEnumeratorSlow):
+        * runtime/VM.h:
+        (JSC::VM::emptyPropertyNameEnumerator):
+
 2019-07-26  Mark Lam  <mark.lam@apple.com>
 
         Add crash diagnostics for debugging unexpected zapped cells.
index d12e2c9..def0b70 100644 (file)
@@ -2162,7 +2162,7 @@ JSCell* JIT_OPERATION operationGetPropertyEnumerator(ExecState* exec, EncodedJSV
 
     JSValue base = JSValue::decode(encodedBase);
     if (base.isUndefinedOrNull())
-        return JSPropertyNameEnumerator::create(vm);
+        return vm.emptyPropertyNameEnumerator();
 
     JSObject* baseObject = base.toObject(exec);
     RETURN_IF_EXCEPTION(scope, { });
index 5df4f58..19370f7 100644 (file)
@@ -972,7 +972,7 @@ SLOW_PATH_DECL(slow_path_get_property_enumerator)
     auto bytecode = pc->as<OpGetPropertyEnumerator>();
     JSValue baseValue = GET(bytecode.m_base).jsValue();
     if (baseValue.isUndefinedOrNull())
-        RETURN(JSPropertyNameEnumerator::create(vm));
+        RETURN(vm.emptyPropertyNameEnumerator());
 
     JSObject* base = baseValue.toObject(exec);
     CHECK_EXCEPTION();
index f862ed4..a6136cd 100644 (file)
@@ -34,65 +34,53 @@ namespace JSC {
 
 const ClassInfo JSPropertyNameEnumerator::s_info = { "JSPropertyNameEnumerator", nullptr, nullptr, nullptr, CREATE_METHOD_TABLE(JSPropertyNameEnumerator) };
 
-JSPropertyNameEnumerator* JSPropertyNameEnumerator::create(VM& vm)
-{
-    if (!vm.emptyPropertyNameEnumerator.get()) {
-        PropertyNameArray propertyNames(&vm, PropertyNameMode::Strings, PrivateSymbolMode::Exclude);
-        vm.emptyPropertyNameEnumerator = Strong<JSCell>(vm, create(vm, 0, 0, 0, WTFMove(propertyNames)));
-    }
-    return jsCast<JSPropertyNameEnumerator*>(vm.emptyPropertyNameEnumerator.get());
-}
-
 JSPropertyNameEnumerator* JSPropertyNameEnumerator::create(VM& vm, Structure* structure, uint32_t indexedLength, uint32_t numberStructureProperties, PropertyNameArray&& propertyNames)
 {
-    StructureID structureID = structure ? structure->id() : 0;
-    uint32_t inlineCapacity = structure ? structure->inlineCapacity() : 0;
-    JSPropertyNameEnumerator* enumerator = new (NotNull, 
-        allocateCell<JSPropertyNameEnumerator>(vm.heap)) JSPropertyNameEnumerator(vm, structureID, inlineCapacity);
-    enumerator->finishCreation(vm, indexedLength, numberStructureProperties, propertyNames.releaseData());
+    unsigned propertyNamesSize = propertyNames.size();
+    unsigned propertyNamesBufferSizeInBytes = (Checked<unsigned>(propertyNamesSize) * sizeof(WriteBarrier<JSString>)).unsafeGet();
+    WriteBarrier<JSString>* propertyNamesBuffer = nullptr;
+    if (propertyNamesBufferSizeInBytes) {
+        propertyNamesBuffer = static_cast<WriteBarrier<JSString>*>(vm.jsValueGigacageAuxiliarySpace.allocateNonVirtual(vm, propertyNamesBufferSizeInBytes, nullptr, AllocationFailureMode::Assert));
+        for (unsigned i = 0; i < propertyNamesSize; ++i)
+            propertyNamesBuffer[i].clear();
+    }
+    JSPropertyNameEnumerator* enumerator = new (NotNull, allocateCell<JSPropertyNameEnumerator>(vm.heap)) JSPropertyNameEnumerator(vm, structure, indexedLength, numberStructureProperties, propertyNamesBuffer, propertyNamesSize);
+    enumerator->finishCreation(vm, propertyNames.releaseData());
     return enumerator;
 }
 
-JSPropertyNameEnumerator::JSPropertyNameEnumerator(VM& vm, StructureID structureID, uint32_t inlineCapacity)
+JSPropertyNameEnumerator::JSPropertyNameEnumerator(VM& vm, Structure* structure, uint32_t indexedLength, uint32_t numberStructureProperties, WriteBarrier<JSString>* propertyNamesBuffer, unsigned propertyNamesSize)
     : JSCell(vm, vm.propertyNameEnumeratorStructure.get())
-    , m_cachedStructureID(structureID)
-    , m_cachedInlineCapacity(inlineCapacity)
+    , m_propertyNames(vm, this, propertyNamesBuffer)
+    , m_cachedStructureID(structure ? structure->id() : 0)
+    , m_indexedLength(indexedLength)
+    , m_endStructurePropertyIndex(numberStructureProperties)
+    , m_endGenericPropertyIndex(propertyNamesSize)
+    , m_cachedInlineCapacity(structure ? structure->inlineCapacity() : 0)
 {
 }
 
-void JSPropertyNameEnumerator::finishCreation(VM& vm, uint32_t indexedLength, uint32_t endStructurePropertyIndex, RefPtr<PropertyNameArrayData>&& identifiers)
+void JSPropertyNameEnumerator::finishCreation(VM& vm, RefPtr<PropertyNameArrayData>&& identifiers)
 {
     Base::finishCreation(vm);
 
     PropertyNameArrayData::PropertyNameVector& vector = identifiers->propertyNameVector();
-
-    m_indexedLength = indexedLength;
-    m_endStructurePropertyIndex = endStructurePropertyIndex;
-    m_endGenericPropertyIndex = vector.size();
-
-    {
-        auto locker = lockDuringMarking(vm.heap, cellLock());
-        m_propertyNames.resizeToFit(vector.size());
-    }
+    ASSERT(m_endGenericPropertyIndex == vector.size());
     for (unsigned i = 0; i < vector.size(); ++i) {
         const Identifier& identifier = vector[i];
-        m_propertyNames[i].set(vm, this, jsString(&vm, identifier.string()));
+        m_propertyNames.get()[i].set(vm, this, jsString(&vm, identifier.string()));
     }
 }
 
-void JSPropertyNameEnumerator::destroy(JSCell* cell)
-{
-    static_cast<JSPropertyNameEnumerator*>(cell)->JSPropertyNameEnumerator::~JSPropertyNameEnumerator();
-}
-
 void JSPropertyNameEnumerator::visitChildren(JSCell* cell, SlotVisitor& visitor)
 {
     JSPropertyNameEnumerator* thisObject = jsCast<JSPropertyNameEnumerator*>(cell);
     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
     Base::visitChildren(cell, visitor);
-    auto locker = holdLock(thisObject->cellLock());
-    for (auto& propertyName : thisObject->m_propertyNames)
-        visitor.append(propertyName);
+    if (auto* propertyNames = thisObject->m_propertyNames.get()) {
+        visitor.markAuxiliary(propertyNames);
+        visitor.append(propertyNames, propertyNames + thisObject->sizeOfPropertyNames());
+    }
     visitor.append(thisObject->m_prototypeChain);
 
     if (thisObject->cachedStructureID()) {
index 4608c63..1079304 100644 (file)
@@ -34,15 +34,11 @@ namespace JSC {
 
 class JSPropertyNameEnumerator final : public JSCell {
 public:
-    typedef JSCell Base;
+    using Base = JSCell;
     static const unsigned StructureFlags = Base::StructureFlags | StructureIsImmortal;
 
-    static JSPropertyNameEnumerator* create(VM&);
     static JSPropertyNameEnumerator* create(VM&, Structure*, uint32_t, uint32_t, PropertyNameArray&&);
 
-    static const bool needsDestruction = true;
-    static void destroy(JSCell*);
-
     static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
     {
         return Structure::create(vm, globalObject, prototype, TypeInfo(CellType, StructureFlags), info());
@@ -50,11 +46,11 @@ public:
 
     DECLARE_EXPORT_INFO;
 
-    JSString* propertyNameAtIndex(uint32_t index) const
+    JSString* propertyNameAtIndex(uint32_t index)
     {
-        if (index >= m_propertyNames.size())
+        if (index >= sizeOfPropertyNames())
             return nullptr;
-        return m_propertyNames[index].get();
+        return m_propertyNames.get()[index].get();
     }
 
     StructureChain* cachedPrototypeChain() const { return m_prototypeChain.get(); }
@@ -71,6 +67,7 @@ public:
     uint32_t endStructurePropertyIndex() const { return m_endStructurePropertyIndex; }
     uint32_t endGenericPropertyIndex() const { return m_endGenericPropertyIndex; }
     uint32_t cachedInlineCapacity() const { return m_cachedInlineCapacity; }
+    uint32_t sizeOfPropertyNames() const { return endGenericPropertyIndex(); }
     static ptrdiff_t cachedStructureIDOffset() { return OBJECT_OFFSETOF(JSPropertyNameEnumerator, m_cachedStructureID); }
     static ptrdiff_t indexedLengthOffset() { return OBJECT_OFFSETOF(JSPropertyNameEnumerator, m_indexedLength); }
     static ptrdiff_t endStructurePropertyIndexOffset() { return OBJECT_OFFSETOF(JSPropertyNameEnumerator, m_endStructurePropertyIndex); }
@@ -78,18 +75,18 @@ public:
     static ptrdiff_t cachedInlineCapacityOffset() { return OBJECT_OFFSETOF(JSPropertyNameEnumerator, m_cachedInlineCapacity); }
     static ptrdiff_t cachedPropertyNamesVectorOffset()
     {
-        return OBJECT_OFFSETOF(JSPropertyNameEnumerator, m_propertyNames) + Vector<WriteBarrier<JSString>>::dataMemoryOffset();
+        return OBJECT_OFFSETOF(JSPropertyNameEnumerator, m_propertyNames);
     }
 
     static void visitChildren(JSCell*, SlotVisitor&);
 
 private:
-    JSPropertyNameEnumerator(VM&, StructureID, uint32_t);
-    void finishCreation(VM&, uint32_t, uint32_t, RefPtr<PropertyNameArrayData>&&);
+    JSPropertyNameEnumerator(VM&, Structure*, uint32_t, uint32_t, WriteBarrier<JSString>*, unsigned);
+    void finishCreation(VM&, RefPtr<PropertyNameArrayData>&&);
 
-    Vector<WriteBarrier<JSString>> m_propertyNames;
-    StructureID m_cachedStructureID;
+    AuxiliaryBarrier<WriteBarrier<JSString>*> m_propertyNames;
     WriteBarrier<StructureChain> m_prototypeChain;
+    StructureID m_cachedStructureID;
     uint32_t m_indexedLength;
     uint32_t m_endStructurePropertyIndex;
     uint32_t m_endGenericPropertyIndex;
index c958740..c039180 100644 (file)
@@ -1308,6 +1308,15 @@ JSCell* VM::sentinelMapBucketSlow()
     return sentinel;
 }
 
+JSPropertyNameEnumerator* VM::emptyPropertyNameEnumeratorSlow()
+{
+    ASSERT(!m_emptyPropertyNameEnumerator);
+    PropertyNameArray propertyNames(this, PropertyNameMode::Strings, PrivateSymbolMode::Exclude);
+    auto* enumerator = JSPropertyNameEnumerator::create(*this, nullptr, 0, 0, WTFMove(propertyNames));
+    m_emptyPropertyNameEnumerator.set(*this, enumerator);
+    return enumerator;
+}
+
 JSGlobalObject* VM::vmEntryGlobalObject(const CallFrame* callFrame) const
 {
     if (callFrame && callFrame->isGlobalExec()) {
index 82462e1..690040d 100644 (file)
@@ -123,6 +123,7 @@ class JSCustomGetterSetterFunction;
 class JSDestructibleObjectHeapCellType;
 class JSGlobalObject;
 class JSObject;
+class JSPropertyNameEnumerator;
 class JSRunLoopTimer;
 class JSStringHeapCellType;
 class JSWebAssemblyCodeBlockHeapCellType;
@@ -545,7 +546,7 @@ public:
     Strong<Structure> m_setIteratorStructure;
     Strong<Structure> m_mapIteratorStructure;
 
-    Strong<JSCell> emptyPropertyNameEnumerator;
+    Strong<JSPropertyNameEnumerator> m_emptyPropertyNameEnumerator;
 
     Strong<JSCell> m_sentinelSetBucket;
     Strong<JSCell> m_sentinelMapBucket;
@@ -598,6 +599,13 @@ public:
         return sentinelMapBucketSlow();
     }
 
+    JSPropertyNameEnumerator* emptyPropertyNameEnumerator()
+    {
+        if (LIKELY(m_emptyPropertyNameEnumerator))
+            return m_emptyPropertyNameEnumerator.get();
+        return emptyPropertyNameEnumeratorSlow();
+    }
+
     WeakGCMap<SymbolImpl*, Symbol, PtrHash<SymbolImpl*>> symbolImplToSymbolMap;
 
     enum class DeletePropertyMode {
@@ -953,6 +961,7 @@ private:
     JS_EXPORT_PRIVATE Structure* mapIteratorStructureSlow();
     JSCell* sentinelSetBucketSlow();
     JSCell* sentinelMapBucketSlow();
+    JSPropertyNameEnumerator* emptyPropertyNameEnumeratorSlow();
 
     void updateStackLimits();