[ic] Fix stores to holey elements

... when the element is read-only in one of the prototypes:
* the length should not be updated,
* in strict mode the store operation should throw TypeError.

Bug: chromium:1055138
Change-Id: I7fc08e22c83f8a9848053cfe20851dc1b82f0e3d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2172090
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67717}

src/codegen/code-stub-assembler.cc

@@ -12658,6 +12658,18 @@ TNode<BoolT> CodeStubAssembler::IsFastElementsKind(
                                Int32Constant(LAST_FAST_ELEMENTS_KIND));
 }
 
+TNode<BoolT> CodeStubAssembler::IsFastOrNonExtensibleOrSealedElementsKind(
+    TNode<Int32T> elements_kind) {
+  STATIC_ASSERT(FIRST_ELEMENTS_KIND == FIRST_FAST_ELEMENTS_KIND);
+  STATIC_ASSERT(LAST_FAST_ELEMENTS_KIND + 1 == PACKED_NONEXTENSIBLE_ELEMENTS);
+  STATIC_ASSERT(PACKED_NONEXTENSIBLE_ELEMENTS + 1 ==
+                HOLEY_NONEXTENSIBLE_ELEMENTS);
+  STATIC_ASSERT(HOLEY_NONEXTENSIBLE_ELEMENTS + 1 == PACKED_SEALED_ELEMENTS);
+  STATIC_ASSERT(PACKED_SEALED_ELEMENTS + 1 == HOLEY_SEALED_ELEMENTS);
+  return Uint32LessThanOrEqual(elements_kind,
+                               Int32Constant(HOLEY_SEALED_ELEMENTS));
+}
+
 TNode<BoolT> CodeStubAssembler::IsDoubleElementsKind(
     TNode<Int32T> elements_kind) {
   STATIC_ASSERT(FIRST_ELEMENTS_KIND == FIRST_FAST_ELEMENTS_KIND);

src/codegen/code-stub-assembler.h

@@ -2742,6 +2742,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   bool IsFastElementsKind(ElementsKind kind) {
     return v8::internal::IsFastElementsKind(kind);
   }
+  TNode<BoolT> IsFastOrNonExtensibleOrSealedElementsKind(
+      TNode<Int32T> elements_kind);
+
   TNode<BoolT> IsDictionaryElementsKind(TNode<Int32T> elements_kind) {
     return ElementsKindEqual(elements_kind, Int32Constant(DICTIONARY_ELEMENTS));
   }

src/ic/ic.cc

@@ -1935,8 +1935,12 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
 Handle<Object> KeyedStoreIC::StoreElementHandler(
     Handle<Map> receiver_map, KeyedAccessStoreMode store_mode,
     MaybeHandle<Object> prev_validity_cell) {
+  // The only case when could keep using non-slow element store handler for
+  // a fast array with potentially read-only elements is when it's an
+  // initializing store to array literal.
   DCHECK_IMPLIES(
-      receiver_map->DictionaryElementsInPrototypeChainOnly(isolate()),
+      !receiver_map->has_dictionary_elements() &&
+          receiver_map->MayHaveReadOnlyElementsInPrototypeChain(isolate()),
       IsStoreInArrayLiteralICKind(kind()));
 
   if (receiver_map->IsJSProxyMap()) {
@@ -2001,7 +2005,7 @@ void KeyedStoreIC::StoreElementPolymorphicHandlers(
     Handle<Map> transition;
 
     if (receiver_map->instance_type() < FIRST_JS_RECEIVER_TYPE ||
-        receiver_map->DictionaryElementsInPrototypeChainOnly(isolate())) {
+        receiver_map->MayHaveReadOnlyElementsInPrototypeChain(isolate())) {
       // TODO(mvstanton): Consider embedding store_mode in the state of the slow
       // keyed store ic for uniformity.
       TRACE_HANDLER_STATS(isolate(), KeyedStoreIC_SlowStub);
@@ -2174,7 +2178,8 @@ MaybeHandle<Object> KeyedStoreIC::Store(Handle<Object> object,
       } else if (key_is_valid_index) {
         if (old_receiver_map->is_abandoned_prototype_map()) {
           set_slow_stub_reason("receiver with prototype map");
-        } else if (!old_receiver_map->DictionaryElementsInPrototypeChainOnly(
+        } else if (old_receiver_map->has_dictionary_elements() ||
+                   !old_receiver_map->MayHaveReadOnlyElementsInPrototypeChain(
                        isolate())) {
           // We should go generic if receiver isn't a dictionary, but our
           // prototype chain does have dictionary elements. This ensures that
@@ -2184,7 +2189,7 @@ MaybeHandle<Object> KeyedStoreIC::Store(Handle<Object> object,
           UpdateStoreElement(old_receiver_map, store_mode,
                              handle(receiver->map(), isolate()));
         } else {
-          set_slow_stub_reason("dictionary or proxy prototype");
+          set_slow_stub_reason("prototype with potentially read-only elements");
         }
       } else {
         set_slow_stub_reason("non-smi-like key");

src/ic/keyed-store-generic.cc

@@ -80,9 +80,9 @@ class KeyedStoreGenericAssembler : public AccessorAssembler {
                              Nothing<LanguageMode>());
   }
 
-  void BranchIfPrototypesHaveNonFastElements(TNode<Map> receiver_map,
-                                             Label* non_fast_elements,
-                                             Label* only_fast_elements);
+  void BranchIfPrototypesMayHaveReadOnlyElements(
+      TNode<Map> receiver_map, Label* maybe_read_only_elements,
+      Label* only_fast_writable_elements);
 
   void TryRewriteElements(TNode<JSObject> receiver, TNode<Map> receiver_map,
                           TNode<FixedArrayBase> elements,
@@ -176,9 +176,9 @@ void KeyedStoreGenericGenerator::SetPropertyInLiteral(
   assembler.SetProperty(context, receiver, key, value, LanguageMode::kStrict);
 }
 
-void KeyedStoreGenericAssembler::BranchIfPrototypesHaveNonFastElements(
-    TNode<Map> receiver_map, Label* non_fast_elements,
-    Label* only_fast_elements) {
+void KeyedStoreGenericAssembler::BranchIfPrototypesMayHaveReadOnlyElements(
+    TNode<Map> receiver_map, Label* maybe_read_only_elements,
+    Label* only_fast_writable_elements) {
   TVARIABLE(Map, var_map);
   var_map = receiver_map;
   Label loop_body(this, &var_map);
@@ -188,16 +188,17 @@ void KeyedStoreGenericAssembler::BranchIfPrototypesHaveNonFastElements(
   {
     TNode<Map> map = var_map.value();
     TNode<HeapObject> prototype = LoadMapPrototype(map);
-    GotoIf(IsNull(prototype), only_fast_elements);
+    GotoIf(IsNull(prototype), only_fast_writable_elements);
     TNode<Map> prototype_map = LoadMap(prototype);
     var_map = prototype_map;
     TNode<Uint16T> instance_type = LoadMapInstanceType(prototype_map);
     GotoIf(IsCustomElementsReceiverInstanceType(instance_type),
-           non_fast_elements);
+           maybe_read_only_elements);
     TNode<Int32T> elements_kind = LoadMapElementsKind(prototype_map);
-    GotoIf(IsFastElementsKind(elements_kind), &loop_body);
+    GotoIf(IsFastOrNonExtensibleOrSealedElementsKind(elements_kind),
+           &loop_body);
     GotoIf(Word32Equal(elements_kind, Int32Constant(NO_ELEMENTS)), &loop_body);
-    Goto(non_fast_elements);
+    Goto(maybe_read_only_elements);
   }
 }
 
@@ -350,8 +351,8 @@ void KeyedStoreGenericAssembler::StoreElementWithCapacity(
               CAST(Load(MachineType::AnyTagged(), elements, offset));
           GotoIf(IsNotTheHole(element), &hole_check_passed);
         }
-        BranchIfPrototypesHaveNonFastElements(receiver_map, slow,
-                                              &hole_check_passed);
+        BranchIfPrototypesMayHaveReadOnlyElements(receiver_map, slow,
+                                                  &hole_check_passed);
         BIND(&hole_check_passed);
       }
     }
@@ -442,7 +443,6 @@ void KeyedStoreGenericAssembler::StoreElementWithCapacity(
     TNode<IntPtrT> offset =
         ElementOffsetFromIndex(index, PACKED_DOUBLE_ELEMENTS, kHeaderSize);
     if (!IsStoreInLiteral()) {
-      // Check if we're about to overwrite the hole. We can safely do that
       // Check if we're about to overwrite the hole. We can safely do that
       // only if there can be no setters on the prototype chain.
       {
@@ -456,8 +456,8 @@ void KeyedStoreGenericAssembler::StoreElementWithCapacity(
           Goto(&hole_check_passed);
           BIND(&found_hole);
         }
-        BranchIfPrototypesHaveNonFastElements(receiver_map, slow,
-                                              &hole_check_passed);
+        BranchIfPrototypesMayHaveReadOnlyElements(receiver_map, slow,
+                                                  &hole_check_passed);
         BIND(&hole_check_passed);
       }
     }

src/objects/elements-kind.h

@@ -127,6 +127,14 @@ inline bool IsDictionaryElementsKind(ElementsKind kind) {
   return kind == DICTIONARY_ELEMENTS;
 }
 
+inline bool IsFastArgumentsElementsKind(ElementsKind kind) {
+  return kind == FAST_SLOPPY_ARGUMENTS_ELEMENTS;
+}
+
+inline bool IsSlowArgumentsElementsKind(ElementsKind kind) {
+  return kind == SLOW_SLOPPY_ARGUMENTS_ELEMENTS;
+}
+
 inline bool IsSloppyArgumentsElementsKind(ElementsKind kind) {
   return base::IsInRange(kind, FAST_SLOPPY_ARGUMENTS_ELEMENTS,
                          SLOW_SLOPPY_ARGUMENTS_ELEMENTS);

src/objects/js-objects-inl.h

@@ -816,7 +816,7 @@ DEF_GETTER(JSObject, HasFastPackedElements, bool) {
 }
 
 DEF_GETTER(JSObject, HasDictionaryElements, bool) {
-  return GetElementsKind(isolate) == DICTIONARY_ELEMENTS;
+  return IsDictionaryElementsKind(GetElementsKind(isolate));
 }
 
 DEF_GETTER(JSObject, HasPackedElements, bool) {
@@ -836,11 +836,11 @@ DEF_GETTER(JSObject, HasNonextensibleElements, bool) {
 }
 
 DEF_GETTER(JSObject, HasFastArgumentsElements, bool) {
-  return GetElementsKind(isolate) == FAST_SLOPPY_ARGUMENTS_ELEMENTS;
+  return IsFastArgumentsElementsKind(GetElementsKind(isolate));
 }
 
 DEF_GETTER(JSObject, HasSlowArgumentsElements, bool) {
-  return GetElementsKind(isolate) == SLOW_SLOPPY_ARGUMENTS_ELEMENTS;
+  return IsSlowArgumentsElementsKind(GetElementsKind(isolate));
 }
 
 DEF_GETTER(JSObject, HasSloppyArgumentsElements, bool) {

src/objects/map.cc

@@ -13,6 +13,7 @@
 #include "src/logging/counters-inl.h"
 #include "src/logging/log.h"
 #include "src/objects/descriptor-array.h"
+#include "src/objects/elements-kind.h"
 #include "src/objects/field-type.h"
 #include "src/objects/js-objects.h"
 #include "src/objects/layout-descriptor.h"
@@ -1418,27 +1419,26 @@ bool Map::OnlyHasSimpleProperties() const {
          !IsSpecialReceiverMap() && !is_dictionary_map();
 }
 
-bool Map::DictionaryElementsInPrototypeChainOnly(Isolate* isolate) {
-  if (IsDictionaryElementsKind(elements_kind())) {
-    return false;
-  }
-
+bool Map::MayHaveReadOnlyElementsInPrototypeChain(Isolate* isolate) {
   for (PrototypeIterator iter(isolate, *this); !iter.IsAtEnd();
        iter.Advance()) {
-    // Be conservative, don't walk into proxies.
-    if (iter.GetCurrent().IsJSProxy()) return true;
-    // String wrappers have non-configurable, non-writable elements.
-    if (iter.GetCurrent().IsStringWrapper()) return true;
-    JSObject current = iter.GetCurrent<JSObject>();
+    // Be conservative, don't look into any JSReceivers that may have custom
+    // elements. For example, into JSProxies, String wrappers (which have have
+    // non-configurable, non-writable elements), API objects, etc.
+    if (iter.GetCurrent().map().IsCustomElementsReceiverMap()) return true;
 
-    if (current.HasDictionaryElements() &&
-        current.element_dictionary().requires_slow_elements()) {
+    JSObject current = iter.GetCurrent<JSObject>();
+    ElementsKind elements_kind = current.GetElementsKind(isolate);
+    if (IsFrozenElementsKind(elements_kind)) return true;
+
+    if (IsDictionaryElementsKind(elements_kind) &&
+        current.element_dictionary(isolate).requires_slow_elements()) {
       return true;
     }
 
-    if (current.HasSlowArgumentsElements()) {
-      FixedArray parameter_map = FixedArray::cast(current.elements());
-      Object arguments = parameter_map.get(1);
+    if (IsSlowArgumentsElementsKind(elements_kind)) {
+      FixedArray parameter_map = FixedArray::cast(current.elements(isolate));
+      Object arguments = parameter_map.get(isolate, 1);
       if (NumberDictionary::cast(arguments).requires_slow_elements()) {
         return true;
       }

src/objects/map.h

@@ -420,9 +420,11 @@ class Map : public HeapObject {
   // there is no guarantee it is attached.
   inline bool IsDetached(Isolate* isolate) const;
 
-  // Returns true if the current map doesn't have DICTIONARY_ELEMENTS but if a
-  // map with DICTIONARY_ELEMENTS was found in the prototype chain.
-  bool DictionaryElementsInPrototypeChainOnly(Isolate* isolate);
+  // Returns true if there is an object with potentially read-only elements
+  // in the prototype chain. It could be a Proxy, a string wrapper,
+  // an object with DICTIONARY_ELEMENTS potentially containing read-only
+  // elements or an object with any frozen elements, or a slow arguments object.
+  bool MayHaveReadOnlyElementsInPrototypeChain(Isolate* isolate);
 
   inline Map ElementsTransitionMap(Isolate* isolate);
 

test/mjsunit/regress/regress-crbug-1055138-1.js

@@ -0,0 +1,64 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+Object.prototype[1] = 153;
+Object.freeze(Object.prototype);
+
+(function TestSloppyStoreToReadOnlyProperty() {
+  function foo() {
+    let ar = [];
+    for (let i = 0; i < 3; i++) {
+      ar[i] = 42;
+
+      if (i == 1) {
+        // Attempt to overwrite read-only element should not change
+        // array length.
+        assertEquals(1, ar.length);
+      } else {
+        assertEquals(i + 1, ar.length);
+      }
+    }
+    return ar;
+  }
+
+  assertEquals([42,153,42], foo());
+  assertEquals([42,153,42], foo());
+  assertEquals([42,153,42], foo());
+  %PrepareFunctionForOptimization(foo);
+  assertEquals([42,153,42], foo());
+  %OptimizeFunctionOnNextCall(foo);
+  assertEquals([42,153,42], foo());
+})();
+
+(function StrictStoreToReadOnlyProperty() {
+  function foo() {
+    "use strict";
+    let ar = [];
+    let threw_exception = false;
+    for (let i = 0; i < 3; i++) {
+      try {
+        ar[i] = 42;
+      } catch(e) {
+        // Attempt to overwrite read-only element should throw and
+        // should not change array length.
+        assertTrue(i == 1);
+        assertEquals(1, ar.length);
+        assertInstanceof(e, TypeError);
+        threw_exception = true;
+      }
+    }
+    assertTrue(threw_exception);
+    return ar;
+  }
+
+  assertEquals([42,153,42], foo());
+  assertEquals([42,153,42], foo());
+  assertEquals([42,153,42], foo());
+  %PrepareFunctionForOptimization(foo);
+  assertEquals([42,153,42], foo());
+  %OptimizeFunctionOnNextCall(foo);
+  assertEquals([42,153,42], foo());
+})();

test/mjsunit/regress/regress-crbug-1055138-2.js

@@ -0,0 +1,34 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+Object.prototype[1] = 153;
+
+(function TestSloppyStoreToReadOnlyProperty() {
+  function foo(prototype_frozen) {
+    let ar = [];
+    for (let i = 0; i < 3; i++) {
+      ar[i] = 42;
+
+      if (prototype_frozen) {
+        if (i == 1) {
+          // Attempt to overwrite read-only element should not change
+          // array length.
+          assertEquals(1, ar.length);
+        } else {
+          assertEquals(i + 1, ar.length);
+        }
+      }
+    }
+    return ar;
+  }
+
+  // Warm-up store IC.
+  assertEquals([42,42,42], foo(false));
+  assertEquals([42,42,42], foo(false));
+  assertEquals([42,42,42], foo(false));
+  assertEquals([42,42,42], foo(false));
+  Object.freeze(Object.prototype);
+  // Ensure IC was properly invalidated.
+  assertEquals([42,153,42], foo(true));
+})();

test/mjsunit/regress/regress-crbug-1055138-3.js

@@ -0,0 +1,40 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+Object.prototype[1] = 153;
+
+(function StrictStoreToReadOnlyProperty() {
+  function foo(prototype_frozen) {
+    "use strict";
+    let ar = [];
+    let threw_exception = false;
+    for (let i = 0; i < 3; i++) {
+      try {
+        ar[i] = 42;
+      } catch(e) {
+        if (prototype_frozen) {
+          // Attempt to overwrite read-only element should throw and
+          // should not change array length.
+          assertTrue(i == 1);
+          assertEquals(1, ar.length);
+          assertInstanceof(e, TypeError);
+          threw_exception = true;
+        }
+      }
+    }
+    if (prototype_frozen) {
+      assertTrue(threw_exception);
+    }
+    return ar;
+  }
+
+  // Warm-up store IC.
+  assertEquals([42,42,42], foo(false));
+  assertEquals([42,42,42], foo(false));
+  assertEquals([42,42,42], foo(false));
+  assertEquals([42,42,42], foo(false));
+  Object.freeze(Object.prototype);
+  // Ensure IC was properly invalidated.
+  assertEquals([42,153,42], foo());
+})();