[builtins] Throw on pop()/shift() when JSArray's length is not writable.

Bug: chromium:823069
Change-Id: Ie5be40da1e64a11c7a3c6ba5d2bc193bd78ca737
Reviewed-on: https://chromium-review.googlesource.com/1002560Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52508}

src/bootstrapper.cc

@@ -1605,6 +1605,7 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,
     PropertyAttributes attribs = static_cast<PropertyAttributes>(
         DONT_ENUM | DONT_DELETE);
 
+    STATIC_ASSERT(JSArray::kLengthDescriptorIndex == 0);
     {  // Add length.
       Descriptor d = Descriptor::AccessorConstant(
           factory->length_string(), factory->array_length_accessor(), attribs);

src/code-stub-assembler.cc

@@ -2244,14 +2244,28 @@ Node* CodeStubAssembler::StoreFeedbackVectorSlot(Node* object,
   }
 }
 
-void CodeStubAssembler::EnsureArrayLengthWritable(Node* map, Label* bailout) {
+void CodeStubAssembler::EnsureArrayLengthWritable(TNode<Map> map,
+                                                  Label* bailout) {
+  // Don't support arrays in dictionary named property mode.
+  GotoIf(IsDictionaryMap(map), bailout);
+
   // Check whether the length property is writable. The length property is the
   // only default named property on arrays. It's nonconfigurable, hence is
   // guaranteed to stay the first property.
-  Node* descriptors = LoadMapDescriptors(map);
-  Node* details =
-      LoadFixedArrayElement(descriptors, DescriptorArray::ToDetailsIndex(0));
-  GotoIf(IsSetSmi(details, PropertyDetails::kAttributesReadOnlyMask), bailout);
+  TNode<DescriptorArray> descriptors = LoadMapDescriptors(map);
+
+  int length_index = JSArray::kLengthDescriptorIndex;
+#ifdef DEBUG
+  TNode<Name> maybe_length = CAST(LoadFixedArrayElement(
+      descriptors, DescriptorArray::ToKeyIndex(length_index)));
+  CSA_ASSERT(this,
+             WordEqual(maybe_length, LoadRoot(Heap::klength_stringRootIndex)));
+#endif
+
+  TNode<Int32T> details = LoadAndUntagToWord32FixedArrayElement(
+      descriptors, DescriptorArray::ToDetailsIndex(length_index));
+  GotoIf(IsSetWord32(details, PropertyDetails::kAttributesReadOnlyMask),
+         bailout);
 }
 
 TNode<Int32T> CodeStubAssembler::EnsureArrayPushable(TNode<Map> map,
@@ -2265,11 +2279,6 @@ TNode<Int32T> CodeStubAssembler::EnsureArrayPushable(TNode<Map> map,
   GotoIf(Word32NotEqual(test, Int32Constant(Map::IsExtensibleBit::kMask)),
          bailout);
 
-  // Disallow pushing onto arrays in dictionary named property mode. We need
-  // to figure out whether the length property is still writable.
-  Comment("Disallow pushing onto arrays in dictionary named property mode");
-  GotoIf(IsDictionaryMap(map), bailout);
-
   EnsureArrayLengthWritable(map, bailout);
 
   TNode<Uint32T> kind = DecodeWord32<Map::ElementsKindBits>(bit_field2);

src/code-stub-assembler.h

@@ -778,7 +778,7 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
       int additional_offset = 0,
       ParameterMode parameter_mode = INTPTR_PARAMETERS);
 
-  void EnsureArrayLengthWritable(Node* map, Label* bailout);
+  void EnsureArrayLengthWritable(TNode<Map> map, Label* bailout);
 
   // EnsureArrayPushable verifies that receiver with this map is:
   //   1. Is not a prototype.

src/objects/js-array.h

@@ -81,6 +81,8 @@ class JSArray : public JSObject {
   static const int kLengthOffset = JSObject::kHeaderSize;
   static const int kSize = kLengthOffset + kPointerSize;
 
+  static const int kLengthDescriptorIndex = 0;
+
   // Max. number of elements being copied in Array builtins.
   static const int kMaxCopyElements = 100;
 

test/mjsunit/regress/regress-crbug-823069.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.
+
+var v = [];
+Object.defineProperty(v, "length", {value: 3, writable: false});
+assertThrows(()=>{v.pop();});
+assertThrows(()=>{v.shift();});