[array] Only use fast-path in Array.p.fill for JSArrays

This CL changes Array.p.fill to use the baseline implementation
for everything other than JSArray.

One of the reasons is that shadowing the length property on
TypedArrays (and other ElementsKinds) is allowed and should be
respected by Array.p.fill. The fast-path for fill for TypedArrays
expects the indices to be clamped to the actual length of the
underlying backing store and not to some length property.

While this mismatch (and others) could probably be handled properly,
we do the conservative thing and only use the fast-path for specific
JSArrays.

R=jgruber@chromium.org

Bug: chromium:865312
Change-Id: Ib3050e3bfc22d47ca8597b6df34788dc2b59b6e1
Reviewed-on: https://chromium-review.googlesource.com/1142772Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Simon Zünd <szuend@google.com>
Cr-Commit-Position: refs/heads/master@{#54558}

src/builtins/builtins-array.cc

@@ -219,28 +219,21 @@ V8_WARN_UNUSED_RESULT bool TryFastArrayFill(
   if (end_index > kMaxUInt32) return false;
   if (!receiver->IsJSObject()) return false;
 
-  Handle<JSObject> object = Handle<JSObject>::cast(receiver);
-
-  if (receiver->IsJSArray()) {
-    if (!EnsureJSArrayWithWritableFastElements(isolate, receiver, args, 1, 1)) {
-      return false;
-    }
+  if (!EnsureJSArrayWithWritableFastElements(isolate, receiver, args, 1, 1)) {
+    return false;
+  }
 
-    Handle<JSArray> array = Handle<JSArray>::cast(receiver);
+  Handle<JSArray> array = Handle<JSArray>::cast(receiver);
 
-    // If no argument was provided, we fill the array with 'undefined'.
-    // EnsureJSArrayWith... does not handle that case so we do it here.
-    // TODO(szuend): Pass target elements kind to EnsureJSArrayWith... when
-    //               it gets refactored.
-    if (args->length() == 1 && array->GetElementsKind() != PACKED_ELEMENTS) {
-      // Use a short-lived HandleScope to avoid creating several copies of the
-      // elements handle which would cause issues when left-trimming later-on.
-      HandleScope scope(isolate);
-      JSObject::TransitionElementsKind(array, PACKED_ELEMENTS);
-    }
-  } else if (!HasOnlySimpleReceiverElements(isolate, *object) ||
-             !object->map()->is_extensible()) {
-    return false;
+  // If no argument was provided, we fill the array with 'undefined'.
+  // EnsureJSArrayWith... does not handle that case so we do it here.
+  // TODO(szuend): Pass target elements kind to EnsureJSArrayWith... when
+  //               it gets refactored.
+  if (args->length() == 1 && array->GetElementsKind() != PACKED_ELEMENTS) {
+    // Use a short-lived HandleScope to avoid creating several copies of the
+    // elements handle which would cause issues when left-trimming later-on.
+    HandleScope scope(isolate);
+    JSObject::TransitionElementsKind(array, PACKED_ELEMENTS);
   }
 
   DCHECK_LE(start_index, kMaxUInt32);
@@ -250,8 +243,8 @@ V8_WARN_UNUSED_RESULT bool TryFastArrayFill(
   CHECK(DoubleToUint32IfEqualToSelf(start_index, &start));
   CHECK(DoubleToUint32IfEqualToSelf(end_index, &end));
 
-  ElementsAccessor* accessor = object->GetElementsAccessor();
-  accessor->Fill(object, value, start, end);
+  ElementsAccessor* accessor = array->GetElementsAccessor();
+  accessor->Fill(array, value, start, end);
   return true;
 }
 }  // namespace

src/elements.cc

@@ -1755,38 +1755,6 @@ class DictionaryElementsAccessor
     return true;
   }
 
-  static Object* FillImpl(Handle<JSObject> receiver, Handle<Object> obj_value,
-                          uint32_t start, uint32_t end) {
-    Isolate* isolate = receiver->GetIsolate();
-    DCHECK(receiver->HasDictionaryElements());
-    Handle<NumberDictionary> dictionary(receiver->element_dictionary(),
-                                        isolate);
-    DCHECK(!dictionary->requires_slow_elements());
-
-    int min_capacity = end - start;
-    if (min_capacity > dictionary->Capacity()) {
-      DictionaryElementsAccessor::GrowCapacityAndConvertImpl(receiver,
-                                                             min_capacity);
-      CHECK(receiver->HasDictionaryElements());
-    }
-
-    for (uint32_t index = start; index < end; ++index) {
-      uint32_t entry = DictionaryElementsAccessor::GetEntryForIndexImpl(
-          isolate, *receiver, receiver->elements(), index, ALL_PROPERTIES);
-      if (entry != kMaxUInt32) {
-        DCHECK_EQ(kData,
-                  DictionaryElementsAccessor::GetDetailsImpl(*dictionary, entry)
-                      .kind());
-        DictionaryElementsAccessor::SetImpl(receiver, entry, *obj_value);
-      } else {
-        // new_capacity parameter is ignored.
-        DictionaryElementsAccessor::AddImpl(receiver, index, obj_value, NONE,
-                                            0);
-      }
-    }
-    return *receiver;
-  }
-
   static Maybe<bool> IncludesValueImpl(Isolate* isolate,
                                        Handle<JSObject> receiver,
                                        Handle<Object> value,
@@ -3106,9 +3074,9 @@ class TypedElementsAccessor
     ctype value = BackingStore::FromHandle(obj_value);
 
     // Ensure indexes are within array bounds
-    DCHECK_LE(0, start);
-    DCHECK_LE(start, end);
-    DCHECK_LE(end, array->length_value());
+    CHECK_LE(0, start);
+    CHECK_LE(start, end);
+    CHECK_LE(end, array->length_value());
 
     DisallowHeapAllocation no_gc;
     BackingStore* elements = BackingStore::cast(receiver->elements());

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

+// Copyright 2018 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.
+
+const intArrayConstructors = [
+  Uint8Array,
+  Int8Array,
+  Uint16Array,
+  Int16Array,
+  Uint32Array,
+  Int32Array,
+  Uint8ClampedArray
+];
+
+const floatArrayConstructors = [
+  Float32Array,
+  Float64Array
+];
+
+const typedArrayConstructors = [...intArrayConstructors,
+                                ...floatArrayConstructors];
+
+for (let constructor of typedArrayConstructors) {
+  // Shadowing the length of a TypedArray should work for Array.p.fill,
+  // but not crash it.
+  let array = new constructor([2, 2]);
+  assertEquals(2, array.length);
+
+  Object.defineProperty(array, 'length', {value: 5});
+  Array.prototype.fill.call(array, 5);
+
+  assertArrayEquals([5, 5], [array[0], array[1]]);
+  assertEquals(undefined, array[2]);
+}