[MERGE #5566 @irinayat-MS] Throw if cannot delete indexed array's property in strict mode

Merge pull request #5566 from irinayat-MS:DeleteFromFrozenArray

Fixes #5564
Another case (array object is either sealed or frozen) when we weren't throwing in strict mode on delete of effectively non-configurable property.

Author: Irina Yatsenko

lib/Runtime/Types/ES5ArrayTypeHandler.cpp

@@ -656,7 +656,14 @@ namespace Js
         // Not in attribute map
         if (!(GetDataItemAttributes() & PropertyConfigurable))
         {
-            return !HasDataItem(arr, index); // CantDelete
+            if (HasDataItem(arr, index))
+            {
+                JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
+                    propertyOperationFlags, instance->GetScriptContext(), TaggedInt::ToString(index, instance->GetScriptContext())->GetString());
+
+                return false;
+            }
+            return true; // non-existing non-configurable property can be deleted
         }
         return arr->DirectDeleteItemAt<Var>(index);
     }

test/Array/delete.js

@@ -19,12 +19,10 @@ var tests = [
             assert.areEqual('whatever', arr.non_indexed, "arr.non_indexed is set to 'whatever'");
             assert.areEqual('with getter', arr.with_getter, "arr.with_getter is set to 'with getter'");
 
-            var res = delete arr.non_indexed;
-            assert.areEqual(true, res, "Deleting own property should succeed");
+            assert.areEqual(true, delete arr.non_indexed, "Deleting own property should succeed");
             assert.areEqual(undefined, arr.non_indexed, "arr.non_indexed has been deleted");
 
-            var res = delete arr.with_getter;
-            assert.areEqual(true, res, "Deleting own property with a getter should succeed");
+            assert.areEqual(true, delete arr.with_getter, "Deleting own property with a getter should succeed");
             assert.areEqual(undefined, arr.with_getter, "arr.with_getter has been deleted");
         }
     },
@@ -35,8 +33,7 @@ var tests = [
             var id = 'id';
             Object.defineProperty(arr, id, { value: 17, configurable: false });
 
-            var res = delete arr[id];
-            assert.areEqual(false, res);
+            assert.areEqual(false, delete arr[id]);
             assert.areEqual(17, arr[id], "arr['id'] value after failed delete");
 
             assert.throws(function () { 'use strict'; delete arr[id]; }, TypeError, 
@@ -49,8 +46,7 @@ var tests = [
         body: function () {
             var arr = [1,4,9,16];
 
-            var res = delete arr.length;
-            assert.areEqual(false, res, "delete of arr.length should fail (as noop)");
+            assert.areEqual(false, delete arr.length, "delete of arr.length should fail (as noop)");
             assert.areEqual(4, arr.length, "arr.length after attempting to delete it");
 
             assert.throws(function () { 'use strict'; delete arr.length; }, TypeError, 
@@ -64,18 +60,15 @@ var tests = [
         body: function () {
             var arr = [1,4,9,16];
 
-            var res = delete arr[1];
-            assert.areEqual(true, res, "delete of arr[1] should succeed");
+            assert.areEqual(true, delete arr[1], "delete of arr[1] should succeed");
             assert.areEqual(undefined, arr[1], "arr[1] value after delete should be undefined");
             assert.areEqual(4, arr.length, "the array's lenght should not change");
 
-            res = delete arr[42];
-            assert.areEqual(true, res, "delete of arr[42] (beyond the array bounds) should succeed");
+            assert.areEqual(true, delete arr[42], "delete of arr[42] (beyond the array bounds) should succeed");
             assert.areEqual(4, arr.length, "the array's length is unchanged");
 
             const last = arr.length - 1;
-            res = delete arr[last];
-            assert.areEqual(true, res, "delete of last element should succeed");
+            assert.areEqual(true, delete arr[last], "delete of last element should succeed");
             assert.areEqual(undefined, arr[last], "arr[last] value after delete should be undefined");
             assert.areEqual(4, arr.length, "the array's lenght should not change");
         }
@@ -87,16 +80,56 @@ var tests = [
             Array.prototype[1]   = "arr.proto";
             var arr = [1,4,9,16,25];
 
-            var res = delete arr[1];
-            assert.areEqual(true, res, "delete of arr[1] should succeed");
+            assert.areEqual(true, delete arr[1], "delete of arr[1] should succeed");
             assert.areEqual("arr.proto", arr[1], "arr[1] after deleting should be picked up from the Array prototype");
 
-            var res = delete arr[4];
-            assert.areEqual(true, res, "delete of arr[4] should succeed");
+            assert.areEqual(true, delete arr[4], "delete of arr[4] should succeed");
             assert.areEqual("obj.proto", arr[4], "arr[4] after deleting should be picked up from the Object prototype");
             assert.areEqual(5, arr.length, "arr.length after deleting of the last element");
         }
     },
+    {
+        name: "Deleting of properties on frozen Arrays",
+        body: function () {
+            var arr = [42];
+            arr.foo = 'fourty-two';
+
+            Object.freeze(arr);
+
+            // indexed property
+            assert.areEqual(false, delete arr[0], "delete arr[0] from frozen array");
+            assert.throws(function () { 'use strict'; delete arr[0]; }, TypeError, 
+                "Should throw on delete of non-indexed property in array", 
+                "Calling delete on '0' is not allowed in strict mode");
+
+            // non-indexed property
+            assert.areEqual(false, delete arr.foo, "delete arr.foo from frozen array");
+            assert.throws(function () { 'use strict'; delete arr.foo; }, TypeError, 
+                "Should throw on delete of non-indexed property in array", 
+                "Calling delete on 'foo' is not allowed in strict mode");
+        }
+    },
+    {
+        name: "Deleting of indexed properties on sealed Arrays",
+        body: function () {
+            var arr = [42];
+            arr.foo = 'fourty-two';
+
+            Object.seal(arr);
+
+            // indexed property
+            assert.areEqual(false, delete arr[0], "delete arr[0] from sealed array");
+            assert.throws(function () { 'use strict'; delete arr[0]; }, TypeError, 
+                "Should throw on delete of non-indexed property in array", 
+                "Calling delete on '0' is not allowed in strict mode");
+
+            // non-indexed property
+            assert.areEqual(false, delete arr.foo, "delete arr.foo from sealed array");
+            assert.throws(function () { 'use strict'; delete arr.foo; }, TypeError, 
+                "Should throw on delete of non-indexed property in array", 
+                "Calling delete on 'foo' is not allowed in strict mode");
+        }
+    },
 ];
 
 testRunner.runTests(tests, { verbose: false /*so no need to provide baseline*/ });