ValueSerializer: Distinguish between 'undefined' and an absent property.

Dealing with this case requires a wire format change. It is possible that an
element can be absent even in an array where the dense format was chosen
(because the array initially had no holes), if the elements are modified while
they are being serialized. In this case, a new tag for the "hole" is emitted.

The logic to treat undefined in dense arrays as an absent property is restricted
to versions of the wire format that this tag did not exist.

BUG=chromium:686159,chromium:665820

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

src/value-serializer.cc

@@ -25,7 +25,8 @@ namespace internal {
 
 // Version 9: (imported from Blink)
 // Version 10: one-byte (Latin-1) strings
-static const uint32_t kLatestVersion = 10;
+// Version 11: properly separate undefined from the hole in arrays
+static const uint32_t kLatestVersion = 11;
 
 static const int kPretenureThreshold = 100 * KB;
 
@@ -49,6 +50,7 @@ enum class SerializationTag : uint8_t {
   // refTableSize:uint32_t (previously used for sanity checks; safe to ignore)
   kVerifyObjectCount = '?',
   // Oddballs (no data).
+  kTheHole = '-',
   kUndefined = '_',
   kNull = '0',
   kTrue = 'T',
@@ -538,10 +540,6 @@ Maybe<bool> ValueSerializer::WriteJSArray(Handle<JSArray> array) {
 
   if (should_serialize_densely) {
     DCHECK_LE(length, static_cast<uint32_t>(FixedArray::kMaxLength));
-
-    // TODO(jbroman): Distinguish between undefined and a hole (this can happen
-    // if serializing one of the elements deletes another). This requires wire
-    // format changes.
     WriteTag(SerializationTag::kBeginDenseJSArray);
     WriteVarint<uint32_t>(length);
     uint32_t i = 0;
@@ -589,6 +587,13 @@ Maybe<bool> ValueSerializer::WriteJSArray(Handle<JSArray> array) {
       // with.
       Handle<Object> element;
       LookupIterator it(isolate_, array, i, array, LookupIterator::OWN);
+      if (!it.IsFound()) {
+        // This can happen in the case where an array that was originally dense
+        // became sparse during serialization. It's too late to switch to the
+        // sparse format, but we can mark the elements as absent.
+        WriteTag(SerializationTag::kTheHole);
+        continue;
+      }
       if (!Object::GetProperty(&it).ToHandle(&element) ||
           !WriteObject(element).FromMaybe(false)) {
         return Nothing<bool>();
@@ -1320,10 +1325,20 @@ MaybeHandle<JSArray> ValueDeserializer::ReadDenseJSArray() {
 
   Handle<FixedArray> elements(FixedArray::cast(array->elements()), isolate_);
   for (uint32_t i = 0; i < length; i++) {
+    SerializationTag tag;
+    if (PeekTag().To(&tag) && tag == SerializationTag::kTheHole) {
+      ConsumeTag(SerializationTag::kTheHole);
+      continue;
+    }
+
     Handle<Object> element;
     if (!ReadObject().ToHandle(&element)) return MaybeHandle<JSArray>();
-    // TODO(jbroman): Distinguish between undefined and a hole.
-    if (element->IsUndefined(isolate_)) continue;
+
+    // Serialization versions less than 11 encode the hole the same as
+    // undefined. For consistency with previous behavior, store these as the
+    // hole. Past version 11, undefined means undefined.
+    if (version_ < 11 && element->IsUndefined(isolate_)) continue;
+
     elements->set(i, *element);
   }
 

test/unittests/value-serializer-unittest.cc

@@ -1245,6 +1245,36 @@ TEST_F(ValueSerializerTest, DecodeSparseArrayVersion0) {
       });
 }
 
+TEST_F(ValueSerializerTest, RoundTripDenseArrayContainingUndefined) {
+  // In previous serialization versions, this would be interpreted as an absent
+  // property.
+  RoundTripTest("[undefined]", [this](Local<Value> value) {
+    ASSERT_TRUE(value->IsArray());
+    EXPECT_EQ(1u, Array::Cast(*value)->Length());
+    EXPECT_TRUE(EvaluateScriptForResultBool("result.hasOwnProperty(0)"));
+    EXPECT_TRUE(EvaluateScriptForResultBool("result[0] === undefined"));
+  });
+}
+
+TEST_F(ValueSerializerTest, DecodeDenseArrayContainingUndefined) {
+  // In previous versions, "undefined" in a dense array signified absence of the
+  // element (for compatibility). In new versions, it has a separate encoding.
+  DecodeTest({0xff, 0x09, 0x41, 0x01, 0x5f, 0x24, 0x00, 0x01},
+             [this](Local<Value> value) {
+               EXPECT_TRUE(EvaluateScriptForResultBool("!(0 in result)"));
+             });
+  DecodeTest(
+      {0xff, 0x0b, 0x41, 0x01, 0x5f, 0x24, 0x00, 0x01},
+      [this](Local<Value> value) {
+        EXPECT_TRUE(EvaluateScriptForResultBool("0 in result"));
+        EXPECT_TRUE(EvaluateScriptForResultBool("result[0] === undefined"));
+      });
+  DecodeTest({0xff, 0x0b, 0x41, 0x01, 0x2d, 0x24, 0x00, 0x01},
+             [this](Local<Value> value) {
+               EXPECT_TRUE(EvaluateScriptForResultBool("!(0 in result)"));
+             });
+}
+
 TEST_F(ValueSerializerTest, RoundTripDate) {
   RoundTripTest("new Date(1e6)", [](Local<Value> value) {
     ASSERT_TRUE(value->IsDate());