[array] Throw TypeError for read-only properties on fast-path

This CL changes the NumberDictionary fast-path for Array.p.sort to
throw a TypeError when trying to write to a read-only property.

Previously, the fast-path simply bailed to the slow-path which could
swallow the TypeError by accident. I.e. because the fast-path could
leave the array in an inconsistent state that is already sorted.

Example:

let arr = new Array(10);
Object.defineProperty(arr, 0, {value: 2, writable: false});
Object.defineProperty(arr, 2, {value: 1, writable: false});
arr.sort();

The pre-processing step will move the value 1 to index 1: {0: 2, 1: 1}
When trying to swap those 2 values, the fast-path will write the 2 at
index 1, then try to write the 1 at index 0 and fail, bailing to the
slow-path. As the array looks like {0: 2, 1: 2} its already sorted
and the TypeError will not be thrown.

R=jgruber@chromium.org

Bug: v8:7382, v8:7907
Change-Id: I5d2f2d73478fdca066ce1048dcb2b8301751cb1f
Reviewed-on: https://chromium-review.googlesource.com/1122120
Commit-Queue: Simon Zünd <szuend@google.com>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54150}

src/builtins/array-sort.tq

@@ -50,28 +50,32 @@ module array {
   }
 
   macro Store<ElementsAccessor : type>(
-      context: Context, receiver: Object, index: Smi, value: Object)
-      labels Bailout {
+      context: Context, receiver: Object, elements: Object, index: Smi,
+      value: Object)
+  labels Bailout {
     SetProperty(context, receiver, index, value, kStrict);
   }
 
   Store<FastPackedSmiElements>(
-      context: Context, elements: Object, index: Smi, value: Object)
-      labels Bailout {
+      context: Context, receiver: Object, elements: Object, index: Smi,
+      value: Object)
+  labels Bailout {
     let elems: FixedArray = unsafe_cast<FixedArray>(elements);
     elems[index] = value;
   }
 
   Store<FastSmiOrObjectElements>(
-      context: Context, elements: Object, index: Smi, value: Object)
-      labels Bailout {
+      context: Context, receiver: Object, elements: Object, index: Smi,
+      value: Object)
+  labels Bailout {
     let elems: FixedArray = unsafe_cast<FixedArray>(elements);
     elems[index] = value;
   }
 
   Store<FastDoubleElements>(
-      context: Context, elements: Object, index: Smi, value: Object)
-      labels Bailout {
+      context: Context, receiver: Object, elements: Object, index: Smi,
+      value: Object)
+  labels Bailout {
     let elems: FixedDoubleArray = unsafe_cast<FixedDoubleArray>(elements);
     let heap_val: HeapNumber = unsafe_cast<HeapNumber>(value);
     // Make sure we do not store signalling NaNs into double arrays.
@@ -86,12 +90,19 @@ module array {
   }
 
   Store<DictionaryElements>(
-      context: Context, elements: Object, index: Smi, value: Object)
-      labels Bailout {
+      context: Context, receiver: Object, elements: Object, index: Smi,
+      value: Object)
+  labels Bailout {
     let dictionary: NumberDictionary = unsafe_cast<NumberDictionary>(elements);
     let intptr_index: intptr = convert<intptr>(index);
-    BasicStoreNumberDictionaryElement(dictionary, intptr_index, value)
-    otherwise Bailout, Bailout;
+    try {
+      BasicStoreNumberDictionaryElement(dictionary, intptr_index, value)
+      otherwise Bailout, Bailout, ReadOnly;
+    }
+    label ReadOnly {
+      ThrowTypeError(context, kStrictReadOnlyProperty, index, Typeof(receiver),
+          receiver);
+    }
     return;
   }
 
@@ -207,12 +218,12 @@ module array {
             initialReceiverLength, userCmpFn, sortCompare, tmp, element)
         otherwise Bailout;
         if (order > 0) {
-          Store<E>(context, elements, j + 1, tmp) otherwise Bailout;
+          Store<E>(context, receiver, elements, j + 1, tmp) otherwise Bailout;
         } else {
           break;
         }
       }
-      Store<E>(context, elements, j + 1, element) otherwise Bailout;
+      Store<E>(context, receiver, elements, j + 1, element) otherwise Bailout;
     }
   }
 
@@ -327,11 +338,12 @@ module array {
     }
 
     // v0 <= v1 <= v2.
-    Store<E>(context, elements, from, v0) otherwise Bailout;
-    Store<E>(context, elements, to - 1, v2) otherwise Bailout;
+    Store<E>(context, receiver, elements, from, v0) otherwise Bailout;
+    Store<E>(context, receiver, elements, to - 1, v2) otherwise Bailout;
 
     // Move pivot element to a place on the left.
-    Swap<E>(context, elements, from + 1, third_index, v1) otherwise Bailout;
+    Swap<E>(context, receiver, elements, from + 1, third_index, v1)
+    otherwise Bailout;
     assert(CanUseSameAccessor<E>(
         context, receiver, elements, initialReceiverMap,
         initialReceiverLength));
@@ -342,12 +354,12 @@ module array {
   // elements[indexB] = elements[indexA].
   // elements[indexA] = value.
   macro Swap<E : type>(
-      context: Context, elements: Object, indexA: Smi, indexB: Smi,
-      value: Object)
+      context: Context, receiver: Object, elements: Object, indexA: Smi,
+      indexB: Smi, value: Object)
   labels Bailout {
     let tmp: Object = Load<E>(context, elements, indexA) otherwise Bailout;
-    Store<E>(context, elements, indexB, tmp) otherwise Bailout;
-    Store<E>(context, elements, indexA, value) otherwise Bailout;
+    Store<E>(context, receiver, elements, indexB, tmp) otherwise Bailout;
+    Store<E>(context, receiver, elements, indexA, value) otherwise Bailout;
   }
 
   macro ArrayQuickSortImpl<E : type>(
@@ -396,7 +408,8 @@ module array {
         otherwise Bailout;
 
         if (order < 0) {
-          Swap<E>(context, elements, low_end, idx, element) otherwise Bailout;
+          Swap<E>(context, receiver, elements, low_end, idx, element)
+          otherwise Bailout;
           low_end++;
         } else if (order > 0) {
           let break_for: bool = false;
@@ -426,13 +439,14 @@ module array {
             break;
           }
 
-          Swap<E>(context, elements, high_start, idx, element)
+          Swap<E>(context, receiver, elements, high_start, idx, element)
           otherwise Bailout;
 
           if (order < 0) {
             element = Load<E>(context, elements, idx) otherwise Bailout;
 
-            Swap<E>(context, elements, low_end, idx, element) otherwise Bailout;
+            Swap<E>(context, receiver, elements, low_end, idx, element)
+            otherwise Bailout;
             low_end++;
           }
         }

src/builtins/base.tq

@@ -133,6 +133,8 @@ const kIncompatibleMethodReceiver: constexpr MessageTemplate generates
 'MessageTemplate::kIncompatibleMethodReceiver';
 const kInvalidDataViewAccessorOffset: constexpr MessageTemplate generates
 'MessageTemplate::kInvalidDataViewAccessorOffset';
+const kStrictReadOnlyProperty: constexpr MessageTemplate generates
+'MessageTemplate::kStrictReadOnlyProperty';
 
 const Hole: Oddball generates 'TheHoleConstant()';
 const Null: Oddball generates 'NullConstant()';
@@ -164,6 +166,8 @@ extern macro HasProperty(
 extern macro ThrowRangeError(Context, constexpr MessageTemplate): never;
 extern macro ThrowTypeError(Context, constexpr MessageTemplate): never;
 extern macro ThrowTypeError(Context, constexpr MessageTemplate, Object): never;
+extern macro ThrowTypeError(Context, constexpr MessageTemplate, Object, Object,
+    Object): never;
 extern macro ArraySpeciesCreate(Context, Object, Number): Object;
 extern macro EnsureArrayPushable(Map): ElementsKind labels Bailout;
 
@@ -587,10 +591,9 @@ macro StoreFixedDoubleArrayElementWithSmiIndex(
 }
 
 extern macro BasicLoadNumberDictionaryElement(NumberDictionary, intptr):
-    Object labels NotData,
-    IfHole;
+    Object labels NotData, IfHole;
 extern macro BasicStoreNumberDictionaryElement(NumberDictionary, intptr, Object)
-labels Fail, IfHole;
+labels NotData, IfHole, ReadOnly;
 
 extern macro IsFastElementsKind(ElementsKind): bool;
 extern macro IsDoubleElementsKind(ElementsKind): bool;
@@ -671,6 +674,7 @@ extern macro IsDetachedBuffer(JSArrayBuffer): bool;
 extern macro IsHeapNumber(HeapObject): bool;
 extern macro IsExtensibleMap(Map): bool;
 extern macro IsCustomElementsReceiverInstanceType(int32): bool;
+extern macro Typeof(Object): Object;
 
 // Return true iff number is NaN.
 macro NumberIsNaN(number: Number): bool {

src/code-stub-assembler.cc

@@ -7494,7 +7494,7 @@ TNode<Object> CodeStubAssembler::BasicLoadNumberDictionaryElement(
 
 void CodeStubAssembler::BasicStoreNumberDictionaryElement(
     TNode<NumberDictionary> dictionary, TNode<IntPtrT> intptr_index,
-    TNode<Object> value, Label* fail, Label* if_hole) {
+    TNode<Object> value, Label* not_data, Label* if_hole, Label* read_only) {
   TVARIABLE(IntPtrT, var_entry);
   Label if_found(this);
   NumberDictionaryLookup(dictionary, intptr_index, &if_found, &var_entry,
@@ -7507,10 +7507,11 @@ void CodeStubAssembler::BasicStoreNumberDictionaryElement(
       LoadDetailsByKeyIndex<NumberDictionary>(dictionary, index);
   TNode<Uint32T> kind = DecodeWord32<PropertyDetails::KindField>(details);
   // TODO(jkummerow): Support accessors without missing?
-  GotoIfNot(Word32Equal(kind, Int32Constant(kData)), fail);
+  GotoIfNot(Word32Equal(kind, Int32Constant(kData)), not_data);
 
   // Check that the property is writeable.
-  GotoIf(IsSetWord32(details, PropertyDetails::kAttributesReadOnlyMask), fail);
+  GotoIf(IsSetWord32(details, PropertyDetails::kAttributesReadOnlyMask),
+         read_only);
 
   // Finally, store the value.
   StoreValueByKeyIndex<NumberDictionary>(dictionary, index, value);

src/code-stub-assembler.h

@@ -2172,8 +2172,8 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
       Label* not_data, Label* if_hole);
   void BasicStoreNumberDictionaryElement(TNode<NumberDictionary> dictionary,
                                          TNode<IntPtrT> intptr_index,
-                                         TNode<Object> value, Label* fail,
-                                         Label* if_hole);
+                                         TNode<Object> value, Label* not_data,
+                                         Label* if_hole, Label* read_only);
 
   template <class Dictionary>
   void FindInsertionEntry(TNode<Dictionary> dictionary, TNode<Name> key,

test/mjsunit/regress/regress-crbug-7907.js

@@ -6,4 +6,4 @@ let arr = new Array(10);
 Object.defineProperty(arr, 0, {value: 10, writable: false});
 Object.defineProperty(arr, 9, {value: 1, writable: false});
 
-arr.sort();
+assertThrows(() => arr.sort(), TypeError);