[JSC] Array.prototype.reverse modifies JSImmutableButterfly

https://bugs.webkit.org/show_bug.cgi?id=188794

Reviewed by Saam Barati.

JSTests:

* stress/reverse-with-immutable-butterfly.js: Added.
(shouldBe):
(reverseInt):
(reverseDouble):
(reverseContiguous):

Source/JavaScriptCore:

While Array.prototype.reverse modifies the butterfly of the given Array,
it does not account JSImmutableButterfly case. So it accidentally modifies
the content of JSImmutableButterfly.
This patch converts CoW arrays to writable arrays before reversing.

* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncReverse):
* runtime/JSObject.h:
(JSC::JSObject::ensureWritable):

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

diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index a19ab57..47100db 100644 (file)
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,16 @@
+2018-08-24  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
+
+        [JSC] Array.prototype.reverse modifies JSImmutableButterfly
+        https://bugs.webkit.org/show_bug.cgi?id=188794
+
+        Reviewed by Saam Barati.
+
+        * stress/reverse-with-immutable-butterfly.js: Added.
+        (shouldBe):
+        (reverseInt):
+        (reverseDouble):
+        (reverseContiguous):
+
 2018-08-22  Saam barati  <sbarati@apple.com>
 
         Make data-view-access.js run less time to prevent timeouts on 32-bit

diff --git a/JSTests/stress/reverse-with-immutable-butterfly.js b/JSTests/stress/reverse-with-immutable-butterfly.js
new file mode 100644 (file)
index 0000000..8f5d428
--- /dev/null
+++ b/JSTests/stress/reverse-with-immutable-butterfly.js
@@ -0,0 +1,28 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+function reverseInt()
+{
+    var array = [0, 1, 2, 3];
+    return array.reverse();
+}
+
+function reverseDouble()
+{
+    var array = [0.0, 1.1, 2.2, 3.3];
+    return array.reverse();
+}
+
+function reverseContiguous()
+{
+    var array = [0.0, 1.1, 2.2, 'hello'];
+    return array.reverse();
+}
+
+for (var i = 0; i < 1e4; ++i) {
+    shouldBe(JSON.stringify(reverseInt()), `[3,2,1,0]`);
+    shouldBe(JSON.stringify(reverseDouble()), `[3.3,2.2,1.1,0]`);
+    shouldBe(JSON.stringify(reverseContiguous()), `["hello",2.2,1.1,0]`);
+}

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 4dcf16a..2d2ac0b 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,20 @@
+2018-08-24  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
+
+        [JSC] Array.prototype.reverse modifies JSImmutableButterfly
+        https://bugs.webkit.org/show_bug.cgi?id=188794
+
+        Reviewed by Saam Barati.
+
+        While Array.prototype.reverse modifies the butterfly of the given Array,
+        it does not account JSImmutableButterfly case. So it accidentally modifies
+        the content of JSImmutableButterfly.
+        This patch converts CoW arrays to writable arrays before reversing.
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::arrayProtoFuncReverse):
+        * runtime/JSObject.h:
+        (JSC::JSObject::ensureWritable):
+
 2018-08-24  Michael Saboff  <msaboff@apple.com>
 
         YARR: Update UCS canonicalization tables for Unicode 11

diff --git a/Source/JavaScriptCore/runtime/ArrayPrototype.cpp b/Source/JavaScriptCore/runtime/ArrayPrototype.cpp
index 764871c..cbf3d72 100644 (file)
--- a/Source/JavaScriptCore/runtime/ArrayPrototype.cpp
+++ b/Source/JavaScriptCore/runtime/ArrayPrototype.cpp
@@ -855,6 +855,8 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncReverse(ExecState* exec)
     unsigned length = toLength(exec, thisObject);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
+    thisObject->ensureWritable(vm);
+
     switch (thisObject->indexingType()) {
     case ALL_CONTIGUOUS_INDEXING_TYPES:
     case ALL_INT32_INDEXING_TYPES: {

diff --git a/Source/JavaScriptCore/runtime/JSArray.cpp b/Source/JavaScriptCore/runtime/JSArray.cpp
index 53b157a..2a6cff8 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSArray.cpp
+++ b/Source/JavaScriptCore/runtime/JSArray.cpp
@@ -287,8 +287,7 @@ bool JSArray::put(JSCell* cell, ExecState* exec, PropertyName propertyName, JSVa
         return ordinarySetSlow(exec, thisObject, propertyName, value, slot.thisValue(), slot.isStrictMode());
     }
 
-    if (isCopyOnWrite(thisObject->indexingMode()))
-        thisObject->convertFromCopyOnWrite(vm);
+    thisObject->ensureWritable(vm);
 
     if (propertyName == vm.propertyNames->length) {
         if (!thisObject->isLengthWritable())
@@ -662,8 +661,7 @@ JSValue JSArray::pop(ExecState* exec)
     VM& vm = exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     Butterfly* butterfly = this->butterfly();
 
@@ -770,14 +768,11 @@ NEVER_INLINE void JSArray::push(ExecState* exec, JSValue value)
 JSArray* JSArray::fastSlice(ExecState& exec, unsigned startIndex, unsigned count)
 {
     VM& vm = exec.vm();
+
+    ensureWritable(vm);
+
     auto arrayType = indexingMode();
     switch (arrayType) {
-    case CopyOnWriteArrayWithInt32:
-    case CopyOnWriteArrayWithDouble:
-    case CopyOnWriteArrayWithContiguous:
-        convertFromCopyOnWrite(vm);
-        arrayType = indexingMode();
-        FALLTHROUGH;
     case ArrayWithDouble:
     case ArrayWithInt32:
     case ArrayWithContiguous: {
@@ -922,8 +917,7 @@ bool JSArray::shiftCountWithAnyIndexingType(ExecState* exec, unsigned& startInde
     VM& vm = exec->vm();
     RELEASE_ASSERT(count > 0);
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     Butterfly* butterfly = this->butterfly();
     
@@ -1081,8 +1075,7 @@ bool JSArray::unshiftCountWithAnyIndexingType(ExecState* exec, unsigned startInd
     VM& vm = exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     Butterfly* butterfly = this->butterfly();
     

diff --git a/Source/JavaScriptCore/runtime/JSArrayInlines.h b/Source/JavaScriptCore/runtime/JSArrayInlines.h
index 1cf0961..368dbd6 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSArrayInlines.h
+++ b/Source/JavaScriptCore/runtime/JSArrayInlines.h
@@ -88,7 +88,8 @@ ALWAYS_INLINE void JSArray::pushInline(ExecState* exec, JSValue value)
     VM& vm = exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-reloop:
+    ensureWritable(vm);
+
     Butterfly* butterfly = this->butterfly();
 
     switch (indexingMode()) {
@@ -228,11 +229,8 @@ reloop:
         return;
     }
 
-    default: {
-        RELEASE_ASSERT(isCopyOnWrite(indexingMode()));
-        convertFromCopyOnWrite(vm);
-        goto reloop;
-    }
+    default:
+        RELEASE_ASSERT_NOT_REACHED();
     }
 }
 

diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp
index db67083..59cfb6d 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSObject.cpp
+++ b/Source/JavaScriptCore/runtime/JSObject.cpp
@@ -838,8 +838,7 @@ bool JSObject::putByIndex(JSCell* cell, ExecState* exec, unsigned propertyName,
         return thisObject->methodTable(vm)->put(thisObject, exec, Identifier::from(exec, propertyName), value, slot);
     }
 
-    if (isCopyOnWrite(thisObject->indexingMode()))
-        thisObject->convertFromCopyOnWrite(vm);
+    thisObject->ensureWritable(vm);
 
     switch (thisObject->indexingType()) {
     case ALL_BLANK_INDEXING_TYPES:
@@ -1636,9 +1635,8 @@ ArrayStorage* JSObject::ensureArrayStorageSlow(VM& vm)
     if (structure(vm)->hijacksIndexingHeader())
         return nullptr;
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
-    
+    ensureWritable(vm);
+
     switch (indexingType()) {
     case ALL_BLANK_INDEXING_TYPES:
         if (UNLIKELY(indexingShouldBeSparse(vm)))
@@ -1673,8 +1671,7 @@ ArrayStorage* JSObject::ensureArrayStorageSlow(VM& vm)
 
 ArrayStorage* JSObject::ensureArrayStorageExistsAndEnterDictionaryIndexingMode(VM& vm)
 {
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     switch (indexingType()) {
     case ALL_BLANK_INDEXING_TYPES: {
@@ -1707,8 +1704,7 @@ ArrayStorage* JSObject::ensureArrayStorageExistsAndEnterDictionaryIndexingMode(V
 
 void JSObject::switchToSlowPutArrayStorage(VM& vm)
 {
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     switch (indexingType()) {
     case ArrayClass:
@@ -2544,8 +2540,7 @@ bool JSObject::defineOwnIndexedProperty(ExecState* exec, unsigned index, const P
 
     ASSERT(index <= MAX_ARRAY_INDEX);
 
-    if (isCopyOnWrite(indexingMode()))
-        convertFromCopyOnWrite(vm);
+    ensureWritable(vm);
 
     if (!inSparseIndexingMode()) {
         // Fast case: we're putting a regular property to a regular array

diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h
index 96ccc04..568b266 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSObject.h
+++ b/Source/JavaScriptCore/runtime/JSObject.h
@@ -865,6 +865,12 @@ public:
 
         return ensureArrayStorageSlow(vm);
     }
+
+    void ensureWritable(VM& vm)
+    {
+        if (isCopyOnWrite(indexingMode()))
+            convertFromCopyOnWrite(vm);
+    }
         
     static size_t offsetOfInlineStorage();