Fix Map::AsArray to properly iterate over the backing store

Old code failed to walk over deleted elements, instead treating
deleted elements as "undefined" in the output array.

This is the Map equivalent of commit 2d9bfe9a.

Also micro-optimized the loops to avoid an extra call to KeyAt()
and used a direct hole comparison instead of calling IsTheHole().

R=cbruni@chromium.org
BUG=v8:4946
LOG=y

Review-Url: https://codereview.chromium.org/1965593002
Cr-Commit-Position: refs/heads/master@{#36149}

src/api.cc

@@ -6367,14 +6367,22 @@ Local<Array> Map::AsArray() const {
   LOG_API(isolate, "Map::AsArray");
   ENTER_V8(isolate);
   i::Handle<i::OrderedHashMap> table(i::OrderedHashMap::cast(obj->table()));
-  int size = table->NumberOfElements();
-  int length = size * 2;
+  int length = table->NumberOfElements() * 2;
   i::Handle<i::FixedArray> result = factory->NewFixedArray(length);
-  for (int i = 0; i < size; ++i) {
-    if (table->KeyAt(i)->IsTheHole()) continue;
-    result->set(i * 2, table->KeyAt(i));
-    result->set(i * 2 + 1, table->ValueAt(i));
+  int result_index = 0;
+  {
+    i::DisallowHeapAllocation no_gc;
+    int capacity = table->UsedCapacity();
+    i::Oddball* the_hole = isolate->heap()->the_hole_value();
+    for (int i = 0; i < capacity; ++i) {
+      i::Object* key = table->KeyAt(i);
+      if (key == the_hole) continue;
+      result->set(result_index++, key);
+      result->set(result_index++, table->ValueAt(i));
+    }
   }
+  DCHECK_EQ(result_index, result->length());
+  DCHECK_EQ(result_index, length);
   i::Handle<i::JSArray> result_array =
       factory->NewJSArrayWithElements(result, i::FAST_ELEMENTS, length);
   return Utils::ToLocal(result_array);
@@ -6451,13 +6459,16 @@ Local<Array> Set::AsArray() const {
   LOG_API(isolate, "Set::AsArray");
   ENTER_V8(isolate);
   i::Handle<i::OrderedHashSet> table(i::OrderedHashSet::cast(obj->table()));
-  int capacity = table->UsedCapacity();
   int length = table->NumberOfElements();
   i::Handle<i::FixedArray> result = factory->NewFixedArray(length);
   int result_index = 0;
-  for (int i = 0; i < capacity; ++i) {
-    i::Object* key = table->KeyAt(i);
-    if (!key->IsTheHole()) {
+  {
+    i::DisallowHeapAllocation no_gc;
+    int capacity = table->UsedCapacity();
+    i::Oddball* the_hole = isolate->heap()->the_hole_value();
+    for (int i = 0; i < capacity; ++i) {
+      i::Object* key = table->KeyAt(i);
+      if (key == the_hole) continue;
       result->set(result_index++, key);
     }
   }

test/cctest/test-api.cc

@@ -24624,7 +24624,7 @@ TEST(SetDeleteThenAsArray) {
   v8::HandleScope handle_scope(isolate);
   LocalContext env;
 
-  // make an array
+  // make a Set
   v8::Local<v8::Value> val = CompileRun("new Set([1, 2, 3])");
   v8::Local<v8::Set> set = v8::Local<v8::Set>::Cast(val);
   CHECK_EQ(3U, set->Size());
@@ -24645,6 +24645,34 @@ TEST(SetDeleteThenAsArray) {
   }
 }
 
+TEST(MapDeleteThenAsArray) {
+  // https://bugs.chromium.org/p/v8/issues/detail?id=4946
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope handle_scope(isolate);
+  LocalContext env;
+
+  // make a Map
+  v8::Local<v8::Value> val = CompileRun("new Map([[1, 2], [3, 4], [5, 6]])");
+  v8::Local<v8::Map> map = v8::Local<v8::Map>::Cast(val);
+  CHECK_EQ(3U, map->Size());
+
+  // delete the "middle" element (using AsArray to
+  // determine which element is the "middle" element)
+  v8::Local<v8::Array> array1 = map->AsArray();
+  CHECK_EQ(6U, array1->Length());
+  // Map::AsArray returns a flat array, so the second key is at index 2.
+  v8::Local<v8::Value> key = array1->Get(env.local(), 2).ToLocalChecked();
+  CHECK(map->Delete(env.local(), key).FromJust());
+
+  // make sure there are no undefined values when we convert to an array again.
+  v8::Local<v8::Array> array2 = map->AsArray();
+  uint32_t length = array2->Length();
+  CHECK_EQ(4U, length);
+  for (uint32_t i = 0; i < length; i++) {
+    CHECK(!array2->Get(env.local(), i).ToLocalChecked()->IsUndefined());
+  }
+}
+
 TEST(CompatibleReceiverCheckOnCachedICHandler) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::HandleScope scope(isolate);