Disallow empty PropertyArray as properties backing store

The only empty PropertyArray is the empty_property_array object on the
isolate. Allowing empty PropertyArrays causes the turbofan to ignore the
existing hash when growing the backing store again. We currently only end
up with the empty PropertyArray when following back transitions.

Bug: chromium:781218, chromium:783713
Change-Id: If41dd09b965cdc8d957b9ca50ba3c8a7f4254769
Reviewed-on: https://chromium-review.googlesource.com/763230
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49318}

src/code-stub-assembler.cc

@@ -2387,6 +2387,7 @@ Node* CodeStubAssembler::AllocateNameDictionary(Node* at_least_space_for) {
 
 Node* CodeStubAssembler::AllocateNameDictionaryWithCapacity(Node* capacity) {
   CSA_ASSERT(this, WordIsPowerOfTwo(capacity));
+  CSA_ASSERT(this, IntPtrGreaterThan(capacity, IntPtrConstant(0)));
   Node* length = EntryToIndex<NameDictionary>(capacity);
   Node* store_size = IntPtrAdd(TimesPointerSize(length),
                                IntPtrConstant(NameDictionary::kHeaderSize));

src/factory.cc

@@ -181,6 +181,7 @@ Handle<FixedArray> Factory::NewFixedArray(int size, PretenureFlag pretenure) {
 Handle<PropertyArray> Factory::NewPropertyArray(int size,
                                                 PretenureFlag pretenure) {
   DCHECK_LE(0, size);
+  if (size == 0) return empty_property_array();
   CALL_HEAP_FUNCTION(isolate(),
                      isolate()->heap()->AllocatePropertyArray(size, pretenure),
                      PropertyArray);
@@ -1331,6 +1332,7 @@ Handle<FixedArray> Factory::CopyFixedArrayAndGrow(Handle<FixedArray> array,
 
 Handle<PropertyArray> Factory::CopyPropertyArrayAndGrow(
     Handle<PropertyArray> array, int grow_by, PretenureFlag pretenure) {
+  DCHECK_LE(0, grow_by);
   CALL_HEAP_FUNCTION(
       isolate(),
       isolate()->heap()->CopyArrayAndGrow(*array, grow_by, pretenure),

src/heap/heap.cc

@@ -3724,7 +3724,10 @@ AllocationResult Heap::AllocateFixedArrayWithFiller(int length,
 
 AllocationResult Heap::AllocatePropertyArray(int length,
                                              PretenureFlag pretenure) {
+  // Allow length = 0 for the empty_property_array singleton.
   DCHECK_LE(0, length);
+  DCHECK_IMPLIES(length == 0, pretenure == TENURED);
+
   DCHECK(!InNewSpace(undefined_value()));
   HeapObject* result = nullptr;
   {

src/objects-debug.cc

@@ -458,6 +458,12 @@ void FixedArray::FixedArrayVerify() {
 }
 
 void PropertyArray::PropertyArrayVerify() {
+  if (length() == 0) {
+    CHECK_EQ(this, this->GetHeap()->empty_property_array());
+    return;
+  }
+  // There are no empty PropertyArrays.
+  CHECK_LT(0, length());
   for (int i = 0; i < length(); i++) {
     Object* e = get(i);
     VerifyPointer(e);

src/objects-printer.cc

@@ -503,7 +503,12 @@ static void JSObjectPrintHeader(std::ostream& os, JSObject* obj,
 
 static void JSObjectPrintBody(std::ostream& os, JSObject* obj,  // NOLINT
                               bool print_elements = true) {
-  os << "\n - properties = " << Brief(obj->raw_properties_or_hash()) << " {";
+  os << "\n - properties = ";
+  Object* properties_or_hash = obj->raw_properties_or_hash();
+  if (!properties_or_hash->IsSmi()) {
+    os << Brief(properties_or_hash);
+  }
+  os << " {";
   if (obj->PrintProperties(os)) os << "\n ";
   os << "}\n";
   if (print_elements && obj->elements()->length() > 0) {

src/objects.cc

@@ -4175,9 +4175,7 @@ void MigrateFastToFast(Handle<JSObject> object, Handle<Map> new_map) {
     }
   }
 
-  if (external > 0) {
-    object->SetProperties(*array);
-  }
+  object->SetProperties(*array);
 
   // Create filler object past the new instance size.
   int new_instance_size = new_map->instance_size();
@@ -4381,6 +4379,10 @@ int Map::NumberOfFields() const {
   return result;
 }
 
+bool Map::HasOutOfObjectProperties() const {
+  return GetInObjectProperties() < NumberOfFields();
+}
+
 void DescriptorArray::GeneralizeAllFields() {
   int length = number_of_descriptors();
   for (int i = 0; i < length; i++) {
@@ -5919,10 +5921,8 @@ void JSObject::AllocateStorageForMap(Handle<JSObject> object, Handle<Map> map) {
       storage = isolate->factory()->NewFixedArray(inobject);
     }
 
-    Handle<PropertyArray> array;
-    if (external > 0) {
-      array = isolate->factory()->NewPropertyArray(external);
-    }
+    Handle<PropertyArray> array =
+        isolate->factory()->NewPropertyArray(external);
 
     for (int i = 0; i < map->NumberOfOwnDescriptors(); i++) {
       PropertyDetails details = descriptors->GetDetails(i);
@@ -5938,9 +5938,7 @@ void JSObject::AllocateStorageForMap(Handle<JSObject> object, Handle<Map> map) {
       }
     }
 
-    if (external > 0) {
-      object->SetProperties(*array);
-    }
+    object->SetProperties(*array);
 
     if (!FLAG_unbox_double_fields) {
       for (int i = 0; i < inobject; i++) {
@@ -6464,13 +6462,16 @@ Object* SetHashAndUpdateProperties(HeapObject* properties, int hash) {
   DCHECK_NE(PropertyArray::kNoHashSentinel, hash);
   DCHECK(PropertyArray::HashField::is_valid(hash));
 
-  if (properties == properties->GetHeap()->empty_fixed_array() ||
-      properties == properties->GetHeap()->empty_property_dictionary()) {
+  Heap* heap = properties->GetHeap();
+  if (properties == heap->empty_fixed_array() ||
+      properties == heap->empty_property_array() ||
+      properties == heap->empty_property_dictionary()) {
     return Smi::FromInt(hash);
   }
 
   if (properties->IsPropertyArray()) {
     PropertyArray::cast(properties)->SetHash(hash);
+    DCHECK_LT(0, PropertyArray::cast(properties)->length());
     return properties;
   }
 
@@ -6522,6 +6523,9 @@ void JSReceiver::SetIdentityHash(int hash) {
 }
 
 void JSReceiver::SetProperties(HeapObject* properties) {
+  DCHECK_IMPLIES(properties->IsPropertyArray() &&
+                     PropertyArray::cast(properties)->length() == 0,
+                 properties == properties->GetHeap()->empty_property_array());
   DisallowHeapAllocation no_gc;
   Isolate* isolate = properties->GetIsolate();
   int hash = GetIdentityHashHelper(isolate, this);

src/objects.h

@@ -1994,6 +1994,9 @@ class JSReceiver: public HeapObject {
   // Gets slow properties for non-global objects.
   inline NameDictionary* property_dictionary() const;
 
+  // Sets the properties backing store and makes sure any existing hash is moved
+  // to the new properties store. To clear out the properties store, pass in the
+  // empty_fixed_array(), the hash will be maintained in this case as well.
   void SetProperties(HeapObject* properties);
 
   // There are five possible values for the properties offset.

src/objects/map.h

@@ -435,6 +435,8 @@ class Map : public HeapObject {
 
   int NumberOfFields() const;
 
+  bool HasOutOfObjectProperties() const;
+
   // Returns true if transition to the given map requires special
   // synchronization with the concurrent marker.
   bool TransitionRequiresSynchronizationWithGC(Map* target) const;

src/runtime/runtime-object.cc

@@ -163,16 +163,23 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
   if (details.location() == kField) {
     isolate->heap()->NotifyObjectLayoutChange(*receiver, map->instance_size(),
                                               no_allocation);
-    Object* filler = isolate->heap()->one_pointer_filler_map();
     FieldIndex index = FieldIndex::ForPropertyIndex(map, details.field_index());
-    JSObject::cast(*receiver)->RawFastPropertyAtPut(index, filler);
-    // We must clear any recorded slot for the deleted property, because
-    // subsequent object modifications might put a raw double there.
-    // Slot clearing is the reason why this entire function cannot currently
-    // be implemented in the DeleteProperty stub.
-    if (index.is_inobject() && !map->IsUnboxedDoubleField(index)) {
-      isolate->heap()->ClearRecordedSlot(
-          *receiver, HeapObject::RawField(*receiver, index.offset()));
+    // Special case deleting the last out-of object property.
+    if (!index.is_inobject() && index.outobject_array_index() == 0) {
+      DCHECK(!Map::cast(backpointer)->HasOutOfObjectProperties());
+      // Clear out the properties backing store.
+      receiver->SetProperties(isolate->heap()->empty_fixed_array());
+    } else {
+      Object* filler = isolate->heap()->one_pointer_filler_map();
+      JSObject::cast(*receiver)->RawFastPropertyAtPut(index, filler);
+      // We must clear any recorded slot for the deleted property, because
+      // subsequent object modifications might put a raw double there.
+      // Slot clearing is the reason why this entire function cannot currently
+      // be implemented in the DeleteProperty stub.
+      if (index.is_inobject() && !map->IsUnboxedDoubleField(index)) {
+        isolate->heap()->ClearRecordedSlot(
+            *receiver, HeapObject::RawField(*receiver, index.offset()));
+      }
     }
   }
   // If the map was marked stable before, then there could be optimized code
@@ -182,6 +189,10 @@ bool DeleteObjectPropertyFast(Isolate* isolate, Handle<JSReceiver> receiver,
   map->NotifyLeafMapLayoutChange();
   // Finally, perform the map rollback.
   receiver->synchronized_set_map(Map::cast(backpointer));
+#if VERIFY_HEAP
+  receiver->HeapObjectVerify();
+  receiver->property_array()->PropertyArrayVerify();
+#endif
   return true;
 }
 

src/runtime/runtime-test.cc

@@ -1087,5 +1087,15 @@ RUNTIME_FUNCTION(Runtime_IsLiftoffFunction) {
   return isolate->heap()->ToBoolean(!wasm_code->is_turbofanned());
 }
 
+RUNTIME_FUNCTION(Runtime_CompleteInobjectSlackTracking) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(1, args.length());
+
+  CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 0);
+  object->map()->CompleteInobjectSlackTracking();
+
+  return isolate->heap()->undefined_value();
+}
+
 }  // namespace internal
 }  // namespace v8

src/runtime/runtime.h

@@ -633,6 +633,7 @@ namespace internal {
   F(WasmNumInterpretedCalls, 1, 1)            \
   F(RedirectToWasmInterpreter, 2, 1)          \
   F(WasmTraceMemory, 4, 1)                    \
+  F(CompleteInobjectSlackTracking, 1, 1)      \
   F(IsLiftoffFunction, 1, 1)
 
 #define FOR_EACH_INTRINSIC_TYPEDARRAY(F) \

test/mjsunit/regress/regress-781218.js

+// Copyright 2017 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
+
+var m = new Map();
+
+function C() { }
+
+// Make sure slack tracking kicks in and shrinks the default size to prevent
+// any further in-object properties.
+%CompleteInobjectSlackTracking(new C());
+
+function f(o) {
+  o.x = true;
+}
+
+// Warm up {f}.
+f(new C());
+f(new C());
+
+
+var o = new C();
+%HeapObjectVerify(o);
+
+m.set(o, 1);  // This creates hash code on o.
+
+// Add an out-of-object property.
+o.x = true;
+%HeapObjectVerify(o);
+// Delete the property (so we have no out-of-object properties).
+delete o.x;
+%HeapObjectVerify(o);
+
+
+// Ensure that growing the properties backing store in optimized code preserves
+// the hash.
+%OptimizeFunctionOnNextCall(f);
+f(o);
+
+%HeapObjectVerify(o);
+assertEquals(1, m.get(o));