Throw when a holey property is set in Array.sort

src/objects.cc

 // Copyright 2015 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.
 
 #include "src/objects.h"
 
 #include <cmath>
 #include <iomanip>
 #include <memory>
 #include <sstream>
@@ skipping 16781 matching lines @@
   // Must stay in dictionary mode, either because of requires_slow_elements,
   // or because we are not going to sort (and therefore compact) all of the
   // elements.
   Handle<SeededNumberDictionary> dict(object->element_dictionary(), isolate);
   Handle<SeededNumberDictionary> new_dict =
       SeededNumberDictionary::New(isolate, dict->NumberOfElements());
 
   uint32_t pos = 0;
   uint32_t undefs = 0;
   int capacity = dict->Capacity();
+  bool is_extensible = JSObject::IsExtensible(object);

[adamk, 2017/02/07 00:23:46]

Please add a DCHECK to JSObject::PrepareElementsForSort, like

DCHECK(JSObject::IsExtensible(object))

This will make sure that even if we in the future can make a non-extensible
object have fast elements, the code will keep working.

[Choongwoo Han, 2017/02/07 09:45:49]

Done.

   Handle<Smi> bailout(Smi::FromInt(-1), isolate);
   // Entry to the new dictionary does not cause it to grow, as we have
   // allocated one that is large enough for all entries.
   DisallowHeapAllocation no_gc;
   for (int i = 0; i < capacity; i++) {
     Object* k = dict->KeyAt(i);
     if (!dict->IsKey(isolate, k)) continue;
 
     DCHECK(k->IsNumber());
     DCHECK(!k->IsSmi() || Smi::cast(k)->value() >= 0);
     DCHECK(!k->IsHeapNumber() || HeapNumber::cast(k)->value() >= 0);
     DCHECK(!k->IsHeapNumber() || HeapNumber::cast(k)->value() <= kMaxUInt32);
 
     HandleScope scope(isolate);
     Handle<Object> value(dict->ValueAt(i), isolate);

[adamk, 2017/02/07 00:23:46]

Rather than changing the logic below, couldn't you simply bail out here if
you've found a hole?

if (value->IsTheHole(isolate)) {
  return bailout;
}

[Choongwoo Han, 2017/02/07 09:45:49]

Since it is a dictionary iteration, value cannot be a hole. We should check if
the new key (pos) is the hole or not. So, I can bail out in two positions like
line 16828 and 16839 did in the original code if we don't want to refactor this
code.

[adamk, 2017/02/08 01:14:07]

Hmm. The duplication doesn't look great either, and both look very tricky.

I'm not sure it's worth tinkering with this code just to support certain cases
that will work. What if we just bail out as soon as we see the object is
non-extensible, like we do for sloppy arguments? That would work just as well,
right?

     PropertyDetails details = dict->DetailsAt(i);
     if (details.kind() == kAccessor || details.IsReadOnly()) {
       // Bail out and do the sorting of undefineds and array holes in JS.
       // Also bail out if the element is not supposed to be moved.
       return bailout;
     }
 
     uint32_t key = NumberToUint32(k);
-    if (key < limit) {
-      if (value->IsUndefined(isolate)) {
-        undefs++;
-      } else if (pos > static_cast<uint32_t>(Smi::kMaxValue)) {
-        // Adding an entry with the key beyond smi-range requires
-        // allocation. Bailout.
-        return bailout;
-      } else {
-        Handle<Object> result = SeededNumberDictionary::AddNumberEntry(
-            new_dict, pos, value, details, object);
-        DCHECK(result.is_identical_to(new_dict));
-        USE(result);
-        pos++;
-      }
-    } else if (key > static_cast<uint32_t>(Smi::kMaxValue)) {
+    uint32_t new_pos = key < limit ? pos : key;
+    if (key < limit && value->IsUndefined(isolate)) {
+      undefs++;
+    } else if (new_pos > static_cast<uint32_t>(Smi::kMaxValue)) {
       // Adding an entry with the key beyond smi-range requires
       // allocation. Bailout.
       return bailout;
+    } else if (!is_extensible && !dict->Has(isolate, new_pos)) {
+      return bailout;
     } else {
       Handle<Object> result = SeededNumberDictionary::AddNumberEntry(
-          new_dict, key, value, details, object);
+          new_dict, new_pos, value, details, object);
       DCHECK(result.is_identical_to(new_dict));
       USE(result);
+      if (key < limit) {
+        pos++;
+      }
     }
   }
 
   uint32_t result = pos;
   PropertyDetails no_details = PropertyDetails::Empty();
   while (undefs > 0) {
     if (pos > static_cast<uint32_t>(Smi::kMaxValue)) {
       // Adding an entry with the key beyond smi-range requires
       // allocation. Bailout.
       return bailout;
@@ skipping 3167 matching lines @@
       // depend on this.
       return DICTIONARY_ELEMENTS;
     }
     DCHECK_LE(kind, LAST_ELEMENTS_KIND);
     return kind;
   }
 }
 
 }  // namespace internal
 }  // namespace v8