Changeset 251399 in webkit

Timestamp:

Oct 21, 2019, 5:04:27 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

Fix issues when setting public length on ArrayWithContiguous type butterflies.
​https://bugs.webkit.org/show_bug.cgi?id=203211
<rdar://problem/56476097>

Reviewed by Keith Miller and Saam Barati.

For ArrayWithContiguous type butterflies, SlotVisitor scans up to the public
length of the butterfly. When setting a new public length, if the new public
length is greater than the current, we should always writeBarrier after the
setting of the new public length. Otherwise, there can be a race where the GC
scans the butterfly after new values have been written to it but before the
public length as been updated. As a result, the new values never get scanned.

For the DFG and FTL, the StoreBarrierInsertionPhase is responsible for inserting
the writeBarriers after the node. Hence, the writeBarrier is guaranteed to be
after the publicLength has been updated.

• runtime/JSArray.cpp:

(JSC::JSArray::shiftCountWithArrayStorage):
(JSC::JSArray::shiftCountWithAnyIndexingType):

• runtime/JSArrayInlines.h:

(JSC::JSArray::pushInline):

• runtime/JSObject.cpp:

(JSC::JSObject::putByIndex):
(JSC::JSObject::reallocateAndShrinkButterfly):

• runtime/JSObject.h:

(JSC::JSObject::setIndexQuickly):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/JSArray.cpp (modified) (2 diffs)
• runtime/JSArrayInlines.h (modified) (1 diff)
• runtime/JSObject.cpp (modified) (2 diffs)
• runtime/JSObject.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-10-21  Mark Lam  <mark.lam@apple.com>
+
+        Fix issues when setting public length on ArrayWithContiguous type butterflies.
+        https://bugs.webkit.org/show_bug.cgi?id=203211
+        <rdar://problem/56476097>
+
+        Reviewed by Keith Miller and Saam Barati.
+
+        For ArrayWithContiguous type butterflies, SlotVisitor scans up to the public
+        length of the butterfly.  When setting a new public length, if the new public
+        length is greater than the current, we should always writeBarrier after the
+        setting of the new public length.  Otherwise, there can be a race where the GC
+        scans the butterfly after new values have been written to it but before the
+        public length as been updated.  As a result, the new values never get scanned.
+
+        For the DFG and FTL, the StoreBarrierInsertionPhase is responsible for inserting
+        the writeBarriers after the node.  Hence, the writeBarrier is guaranteed to be
+        after the publicLength has been updated.
+
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::shiftCountWithArrayStorage):
+        (JSC::JSArray::shiftCountWithAnyIndexingType):
+        * runtime/JSArrayInlines.h:
+        (JSC::JSArray::pushInline):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::putByIndex):
+        (JSC::JSObject::reallocateAndShrinkButterfly):
+        * runtime/JSObject.h:
+        (JSC::JSObject::setIndexQuickly):
+
 2019-10-21  Devin Rousso  <drousso@apple.com>
 

trunk/Source/JavaScriptCore/runtime/JSArray.cpp

@@ -895 +895 @@
     Butterfly* butterfly = this->butterfly();
     
-    switch (indexingType()) {
+    auto indexingType = this->indexingType();
+    switch (indexingType) {
     case ArrayClass:
         return true;
@@ -935 +936 @@
         for (unsigned i = end; i < oldLength; ++i)
             butterfly->contiguous().at(this, i).clear();
-        
+
         butterfly->setPublicLength(oldLength - count);
 
         // Our memmoving of values around in the array could have concealed some of them from
         // the collector. Let's make sure that the collector scans this object again.
-        vm.heap.writeBarrier(this);
-        
+        if (indexingType == ArrayWithContiguous)
+            vm.heap.writeBarrier(this);
+
         return true;
     }

trunk/Source/JavaScriptCore/runtime/JSArrayInlines.h

@@ -158 +158 @@
         ASSERT(length <= butterfly->vectorLength());
         if (length < butterfly->vectorLength()) {
-            butterfly->contiguous().at(this, length).set(vm, this, value);
+            butterfly->contiguous().at(this, length).setWithoutWriteBarrier(value);
             butterfly->setPublicLength(length + 1);
+            vm.heap.writeBarrier(this, value);
             return;
         }

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -892 +892 @@
         if (propertyName >= butterfly->vectorLength())
             break;
-        butterfly->contiguous().at(thisObject, propertyName).set(vm, thisObject, value);
+        butterfly->contiguous().at(thisObject, propertyName).setWithoutWriteBarrier(value);
         if (propertyName >= butterfly->publicLength())
             butterfly->setPublicLength(propertyName + 1);
+        vm.heap.writeBarrier(thisObject, value);
         return true;
     }
@@ -3430 +3431 @@
     ASSERT(hasContiguous(indexingType()) || hasInt32(indexingType()) || hasDouble(indexingType()) || hasUndecided(indexingType()));
     ASSERT(m_butterfly->vectorLength() > length);
+    ASSERT(m_butterfly->publicLength() >= length);
     ASSERT(!m_butterfly->indexingHeader()->preCapacity(structure(vm)));
 

trunk/Source/JavaScriptCore/runtime/JSObject.h

@@ -410 +410 @@
         case ALL_CONTIGUOUS_INDEXING_TYPES: {
             ASSERT(i < butterfly->vectorLength());
-            butterfly->contiguous().at(this, i).set(vm, this, v);
+            butterfly->contiguous().at(this, i).setWithoutWriteBarrier(v);
             if (i >= butterfly->publicLength())
                 butterfly->setPublicLength(i + 1);
+            vm.heap.writeBarrier(this, v);
             break;
         }