[ic] Properly handle store mode generalization in KeyedStoreIC

... when one of the receivers is a JSArray that may have a read-only
length.

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

src/ic/ic.cc

@@ -8,6 +8,7 @@
 #include "src/api/api.h"
 #include "src/ast/ast.h"
 #include "src/base/bits.h"
+#include "src/base/logging.h"
 #include "src/builtins/accessors.h"
 #include "src/codegen/code-factory.h"
 #include "src/execution/arguments-inl.h"
@@ -1856,6 +1857,12 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
     if (receiver_map.is_identical_to(previous_receiver_map) &&
         new_receiver_map.is_identical_to(receiver_map) &&
         old_store_mode == STANDARD_STORE && store_mode != STANDARD_STORE) {
+      if (receiver_map->IsJSArrayMap() &&
+          JSArray::MayHaveReadOnlyLength(*receiver_map)) {
+        set_slow_stub_reason(
+            "can't generalize store mode (potentially read-only length)");
+        return;
+      }
       // A "normal" IC that handles stores can switch to a version that can
       // grow at the end of the array, handle OOB accesses or copy COW arrays
       // and still stay MONOMORPHIC.
@@ -1900,13 +1907,18 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
   }
 
   // If the store mode isn't the standard mode, make sure that all polymorphic
-  // receivers are either external arrays, or all "normal" arrays. Otherwise,
-  // use the megamorphic stub.
+  // receivers are either external arrays, or all "normal" arrays with writable
+  // length. Otherwise, use the megamorphic stub.
   if (store_mode != STANDARD_STORE) {
     size_t external_arrays = 0;
     for (MapAndHandler map_and_handler : target_maps_and_handlers) {
       Handle<Map> map = map_and_handler.first;
-      if (map->has_typed_array_elements()) {
+      if (map->IsJSArrayMap() && JSArray::MayHaveReadOnlyLength(*map)) {
+        set_slow_stub_reason(
+            "unsupported combination of arrays (potentially read-only length)");
+        return;
+
+      } else if (map->has_typed_array_elements()) {
         DCHECK(!IsStoreInArrayLiteralICKind(kind()));
         external_arrays++;
       }

src/objects/js-array.h

@@ -30,6 +30,7 @@ class JSArray : public JSObject {
   // is set to a smi. This matches the set function on FixedArray.
   inline void set_length(Smi length);
 
+  static bool MayHaveReadOnlyLength(Map js_array_map);
   static bool HasReadOnlyLength(Handle<JSArray> array);
   static bool WouldChangeReadOnlyLength(Handle<JSArray> array, uint32_t index);
 

src/objects/objects.cc

@@ -5717,17 +5717,27 @@ const char* AllocationSite::PretenureDecisionName(PretenureDecision decision) {
   return nullptr;
 }
 
+// static
+bool JSArray::MayHaveReadOnlyLength(Map js_array_map) {
+  DCHECK(js_array_map.IsJSArrayMap());
+  if (js_array_map.is_dictionary_map()) return true;
+
+  // Fast path: "length" is the first fast property of arrays with non
+  // dictionary properties. Since it's not configurable, it's guaranteed to be
+  // the first in the descriptor array.
+  InternalIndex first(0);
+  DCHECK(js_array_map.instance_descriptors().GetKey(first) ==
+         js_array_map.GetReadOnlyRoots().length_string());
+  return js_array_map.instance_descriptors().GetDetails(first).IsReadOnly();
+}
+
 bool JSArray::HasReadOnlyLength(Handle<JSArray> array) {
   Map map = array->map();
-  // Fast path: "length" is the first fast property of arrays. Since it's not
-  // configurable, it's guaranteed to be the first in the descriptor array.
-  if (!map.is_dictionary_map()) {
-    InternalIndex first(0);
-    DCHECK(map.instance_descriptors().GetKey(first) ==
-           array->GetReadOnlyRoots().length_string());
-    return map.instance_descriptors().GetDetails(first).IsReadOnly();
-  }
 
+  // If map guarantees that there can't be a read-only length, we are done.
+  if (!MayHaveReadOnlyLength(map)) return false;
+
+  // Look at the object.
   Isolate* isolate = array->GetIsolate();
   LookupIterator it(isolate, array, isolate->factory()->length_string(), array,
                     LookupIterator::OWN_SKIP_INTERCEPTOR);

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

+// 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: --no-lazy-feedback-allocation
+
+function store(ar, index) {
+  ar[index] = "a";
+}
+
+let growable_array = [];
+
+// Train IC on growable array
+store(growable_array, 0);
+store(growable_array, 1);
+store(growable_array, 2);
+store(growable_array, 3);
+
+// Now make IC polymorphic
+var array = [];
+Object.defineProperty(array, "length", { value: 3, writable: false });
+
+store(array, 0);
+store(array, 1);
+
+// ... and try to grow it.
+store(array, 3);
+assertEquals(undefined, array[3]);
+assertEquals(3, array.length);