[ES6] Make Array.prototype.reverse spec compatible.
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 Mar 2016 19:49:36 +0000 (19:49 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 Mar 2016 19:49:36 +0000 (19:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=155528

Reviewed by Michael Saboff.

This patch make Array.prototype.reverse spec compatible.
Before, we weren't performing a HasProperty of each index
before performing a Get on that index.  We now do that on
the slow path.

* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncReverse):
* tests/stress/array-reverse-proxy.js: Added.
(assert):
(test):
(shallowCopy):
(shallowEqual):
(let.handler.get getSet):
(test.let.handler.get getSet):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
Source/JavaScriptCore/tests/es6.yaml
Source/JavaScriptCore/tests/stress/array-reverse-proxy.js [new file with mode: 0644]

index ad60c47..d387091 100644 (file)
@@ -1,3 +1,25 @@
+2016-03-16  Saam Barati  <sbarati@apple.com>
+
+        [ES6] Make Array.prototype.reverse spec compatible.
+        https://bugs.webkit.org/show_bug.cgi?id=155528
+
+        Reviewed by Michael Saboff.
+
+        This patch make Array.prototype.reverse spec compatible.
+        Before, we weren't performing a HasProperty of each index
+        before performing a Get on that index.  We now do that on
+        the slow path.
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::arrayProtoFuncReverse):
+        * tests/stress/array-reverse-proxy.js: Added.
+        (assert):
+        (test):
+        (shallowCopy):
+        (shallowEqual):
+        (let.handler.get getSet):
+        (test.let.handler.get getSet):
+
 2016-03-16  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, rolling out r198235, r198240, r198241, and
index 9a8d197..0681639 100644 (file)
@@ -737,8 +737,9 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncReverse(ExecState* exec)
     if (!thisObject)
         return JSValue::encode(JSValue());
 
+    VM& vm = exec->vm();
     unsigned length = getLength(exec, thisObject);
-    if (exec->hadException())
+    if (vm.exception())
         return JSValue::encode(jsUndefined());
 
     switch (thisObject->indexingType()) {
@@ -776,31 +777,40 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncReverse(ExecState* exec)
     }
 
     unsigned middle = length / 2;
-    for (unsigned k = 0; k < middle; k++) {
-        unsigned lk1 = length - k - 1;
-        JSValue obj2 = getProperty(exec, thisObject, lk1);
-        if (exec->hadException())
-            return JSValue::encode(jsUndefined());
-        JSValue obj = getProperty(exec, thisObject, k);
-        if (exec->hadException())
+    for (unsigned lower = 0; lower < middle; lower++) {
+        unsigned upper = length - lower - 1;
+        bool lowerExists = thisObject->hasProperty(exec, lower);
+        if (vm.exception())
             return JSValue::encode(jsUndefined());
+        JSValue lowerValue;
+        if (lowerExists)
+            lowerValue = thisObject->get(exec, lower);
 
-        if (obj2) {
-            thisObject->putByIndexInline(exec, k, obj2, true);
-            if (exec->hadException())
-                return JSValue::encode(jsUndefined());
-        } else if (!thisObject->methodTable(exec->vm())->deletePropertyByIndex(thisObject, exec, k)) {
-            throwTypeError(exec, ASCIILiteral("Unable to delete property."));
+        bool upperExists = thisObject->hasProperty(exec, upper);
+        if (vm.exception())
             return JSValue::encode(jsUndefined());
+        JSValue upperValue;
+        if (upperExists)
+            upperValue = thisObject->get(exec, upper);
+
+        if (upperExists) {
+            thisObject->putByIndexInline(exec, lower, upperValue, true);
+            if (vm.exception())
+                return JSValue::encode(JSValue());
+        } else if (!thisObject->methodTable(vm)->deletePropertyByIndex(thisObject, exec, lower)) {
+            if (!vm.exception())
+                throwTypeError(exec, ASCIILiteral("Unable to delete property."));
+            return JSValue::encode(JSValue());
         }
 
-        if (obj) {
-            thisObject->putByIndexInline(exec, lk1, obj, true);
-            if (exec->hadException())
-                return JSValue::encode(jsUndefined());
-        } else if (!thisObject->methodTable(exec->vm())->deletePropertyByIndex(thisObject, exec, lk1)) {
-            throwTypeError(exec, ASCIILiteral("Unable to delete property."));
-            return JSValue::encode(jsUndefined());
+        if (lowerExists) {
+            thisObject->putByIndexInline(exec, upper, lowerValue, true);
+            if (vm.exception())
+                return JSValue::encode(JSValue());
+        } else if (!thisObject->methodTable(vm)->deletePropertyByIndex(thisObject, exec, upper)) {
+            if (!vm.exception())
+                throwTypeError(exec, ASCIILiteral("Unable to delete property."));
+            return JSValue::encode(JSValue());
         }
     }
     return JSValue::encode(thisObject);
index 5e5c3f9..2ce84d4 100644 (file)
 - path: es6/Proxy_internal_deleteProperty_calls_Array.prototype.pop.js
   cmd: runES6 :normal
 - path: es6/Proxy_internal_deleteProperty_calls_Array.prototype.reverse.js
-  cmd: runES6 :fail
+  cmd: runES6 :normal
 - path: es6/Proxy_internal_deleteProperty_calls_Array.prototype.shift.js
   cmd: runES6 :fail
 - path: es6/Proxy_internal_deleteProperty_calls_Array.prototype.splice.js
 - path: es6/Proxy_internal_get_calls_Array.prototype.pop.js
   cmd: runES6 :normal
 - path: es6/Proxy_internal_get_calls_Array.prototype.reverse.js
-  cmd: runES6 :fail
+  cmd: runES6 :normal
 - path: es6/Proxy_internal_get_calls_Array.prototype.shift.js
   cmd: runES6 :normal
 - path: es6/Proxy_internal_get_calls_Array.prototype.splice.js
 - path: es6/Proxy_internal_set_calls_Array.prototype.push.js
   cmd: runES6 :normal
 - path: es6/Proxy_internal_set_calls_Array.prototype.reverse.js
-  cmd: runES6 :fail
+  cmd: runES6 :normal
 - path: es6/Proxy_internal_set_calls_Array.prototype.shift.js
   cmd: runES6 :fail
 - path: es6/Proxy_internal_set_calls_Array.prototype.splice.js
diff --git a/Source/JavaScriptCore/tests/stress/array-reverse-proxy.js b/Source/JavaScriptCore/tests/stress/array-reverse-proxy.js
new file mode 100644 (file)
index 0000000..45d843d
--- /dev/null
@@ -0,0 +1,221 @@
+function assert(b) {
+    if (!b)
+        throw new Error("bad assertion!");
+}
+
+function test(f) {
+    for (let i = 0; i < 1000; i++)
+        f();
+}
+
+function shallowCopy(arr) {
+    let result = [];
+    for (let item of arr)
+        result.push(item);
+    return result;
+}
+
+function shallowEqual(a, b) {
+    if (a.length !== b.length)
+        return false;
+    for (let i = 0; i < a.length; i++) {
+        if (a[i] !== b[i])
+            return false;
+    }
+
+    return true;
+}
+
+test(function() {
+    let target = [10, 20, 30, 40];
+    let copy = shallowCopy(target);
+    let handler = { };
+    let proxy = new Proxy(target, handler);
+    proxy.reverse();
+    copy.reverse();
+    assert(shallowEqual(proxy, target));
+    assert(shallowEqual(target, copy));
+});
+
+test(function() {
+    let target = [10, 20, 30, 40];
+    let copy = shallowCopy(target);
+    let getSet = new Set;
+    let hasSet = new Set;
+    let handler = {
+        get(theTarget, key) {
+            getSet.add(key);
+            return theTarget[key];
+        },
+        has(theTarget, key) {
+            hasSet.add(key);
+            return Reflect.has(theTarget, key);
+        }
+    };
+    let proxy = new Proxy(target, handler);
+    proxy.reverse();
+    copy.reverse();
+    assert(shallowEqual(proxy, target));
+    assert(shallowEqual(target, copy));
+
+    assert(getSet.has("0"));
+    assert(getSet.has("1"));
+    assert(getSet.has("2"));
+    assert(getSet.has("3"));
+    assert(getSet.has("length"));
+
+    assert(hasSet.has("0"));
+    assert(hasSet.has("1"));
+    assert(hasSet.has("2"));
+    assert(hasSet.has("3"));
+});
+
+test(function() {
+    let target = [10, 20, 30, 40];
+    let getSet = new Set;
+    let hasSet = new Set;
+    let deleteSet = new Set;
+    let handler = {
+        get(theTarget, key) {
+            getSet.add(key);
+            return theTarget[key];
+        },
+        has(theTarget, key) {
+            hasSet.add(key);
+            if (key === "0" || key === "1")
+                return true;
+            assert(key === "2" || key === "3");
+            return false;
+        },
+        deleteProperty(theTarget, key) {
+            deleteSet.add(key);
+            return Reflect.deleteProperty(theTarget, key);
+        }
+    };
+
+    let proxy = new Proxy(target, handler);
+    proxy.reverse();
+    assert(shallowEqual(proxy, target));
+
+    assert(getSet.has("0"));
+    assert(getSet.has("1"));
+    assert(getSet.has("2"));
+    assert(getSet.has("3"));
+    assert(getSet.has("length"));
+    assert(getSet.has("reverse"));
+    assert(getSet.size === 6);
+
+    assert(hasSet.has("0"));
+    assert(hasSet.has("1"));
+    assert(hasSet.has("2"));
+    assert(hasSet.has("3"));
+    assert(hasSet.size === 4);
+
+    assert(deleteSet.has("0"));
+    assert(deleteSet.has("1"));
+    assert(!deleteSet.has("2"));
+    assert(!deleteSet.has("3"));
+});
+
+test(function() {
+    let target = [10, 20, 30, 40];
+    let error;
+    let handler = {
+        has(theTarget, key) {
+            return false;
+        },
+        deleteProperty(theTarget, key) {
+            if (key === "0") {
+                error = new Error;
+                throw error;
+            }
+            return true;
+        }
+    };
+
+    let proxy = new Proxy(target, handler);
+    let threw = false;
+    try {
+        proxy.reverse();
+    } catch(e) {
+        threw = true;
+        assert(e === error);
+    }
+    assert(threw);
+});
+
+test(function() {
+    let target = [10, 20, 30, 40];
+    let error;
+    let handler = {
+        has(theTarget, key) {
+            if (key === "0") {
+                error = new Error;
+                throw error;
+            }
+            return false;
+        }
+    };
+
+    let proxy = new Proxy(target, handler);
+    let threw = false;
+    try {
+        proxy.reverse();
+    } catch(e) {
+        threw = true;
+        assert(e === error);
+    }
+    assert(threw);
+});
+
+test(function() {
+    let target = [10, 20, 30, 40];
+    let error;
+    let handler = {
+        has(theTarget, key) {
+            if (key === "3") {
+                error = new Error;
+                throw error;
+            }
+            return false;
+        }
+    };
+
+    let proxy = new Proxy(target, handler);
+    let threw = false;
+    try {
+        proxy.reverse();
+    } catch(e) {
+        threw = true;
+        assert(e === error);
+    }
+    assert(threw);
+});
+
+test(function() {
+    let target = [10, 20, 30, 40];
+    let error;
+    let getSet = new Set;
+    let handler = {
+        get(theTarget, key) {
+            getSet.add(key);
+            return theTarget[key];
+        },
+        has(theTarget, key) {
+            return false;
+        },
+        deleteProperty() {
+            return true;
+        }
+    };
+
+    let proxy = new Proxy(target, handler);
+    proxy.reverse();
+    assert(!getSet.has("0"));
+    assert(!getSet.has("1"));
+    assert(!getSet.has("2"));
+    assert(!getSet.has("3"));
+    assert(getSet.size === 2);
+    assert(getSet.has("reverse"));
+    assert(getSet.has("length"));
+});