Blink-compatible serialization of dictionary-like objects.

test/unittests/value-serializer-unittest.cc

Index: test/unittests/value-serializer-unittest.cc
diff --git a/test/unittests/value-serializer-unittest.cc b/test/unittests/value-serializer-unittest.cc
index c48961fb6cddbb61ac4c2fa464af29ab925e0062..2450fa42301a5729620d2cd86953d60d8528504e 100644
--- a/test/unittests/value-serializer-unittest.cc
+++ b/test/unittests/value-serializer-unittest.cc
@@ -47,7 +47,7 @@ class ValueSerializerTest : public TestWithIsolate {
     Local<Value> input_value = input_functor();
     i::Isolate* internal_isolate = reinterpret_cast<i::Isolate*>(isolate());
     i::HandleScope handle_scope(internal_isolate);
-    i::ValueSerializer serializer;
+    i::ValueSerializer serializer(internal_isolate);
     serializer.WriteHeader();
     ASSERT_TRUE(serializer.WriteObject(Utils::OpenHandle(*input_value))
                     .FromMaybe(false));
@@ -388,5 +388,109 @@ TEST_F(ValueSerializerTest, EncodeTwoByteStringUsesPadding) {
       });
 }
 
+TEST_F(ValueSerializerTest, RoundTripDictionaryObject) {
+  // Empty object.
+  RoundTripTest([this]() { return EvaluateScriptForInput("({})"); },
+                [this](Local<Value> value) {
+                  ASSERT_TRUE(value->IsObject());
+                  EXPECT_TRUE(EvaluateScriptForResultBool(
+                      "Object.getPrototypeOf(result) === Object.prototype"));
+                  EXPECT_TRUE(EvaluateScriptForResultBool(
+                      "Object.getOwnPropertyNames(result).length === 0"));
+                });
+  // String key.
+  RoundTripTest(
+      [this]() { return EvaluateScriptForInput("({ a: 42 })"); },
+      [this](Local<Value> value) {
+        ASSERT_TRUE(value->IsObject());
+        EXPECT_TRUE(EvaluateScriptForResultBool("result.hasOwnProperty('a')"));
+        EXPECT_TRUE(EvaluateScriptForResultBool("result.a === 42"));
+        EXPECT_TRUE(EvaluateScriptForResultBool(
+            "Object.getOwnPropertyNames(result).length === 1"));
+      });
+  // Integer key (treated as a string, but may be encoded differently).

[Camillo Bruni, 2016/08/16 11:26:57]

nit: can you add a copy of this test with the corner-case array indices that can
no longer be encoded as Smis?

The properties correspond to the enumeration index:
{a: 2, 0xFFFFFFFE: 1, 1: 0}
vs.
{a: 1, 0xFFFFFFFF: 2, 1: 0}

[jbroman, 2016/08/16 18:16:29]

I've incorporated such an example. It took me awhile to find the spec reference
for why this behaves the way it does (2^31 - 1 is explicitly ruled out as being
considered an index, though I don't understand why).

+  RoundTripTest(
+      [this]() { return EvaluateScriptForInput("({ 42: 'a' })"); },
+      [this](Local<Value> value) {
+        ASSERT_TRUE(value->IsObject());
+        EXPECT_TRUE(EvaluateScriptForResultBool("result.hasOwnProperty('42')"));
+        EXPECT_TRUE(EvaluateScriptForResultBool("result[42] === 'a'"));
+        EXPECT_TRUE(EvaluateScriptForResultBool(
+            "Object.getOwnPropertyNames(result).length === 1"));
+      });
+  // Key order must be preserved.
+  RoundTripTest(
+      [this]() { return EvaluateScriptForInput("({ x: 1, y: 2, a: 3 })"); },
+      [this](Local<Value> value) {
+        EXPECT_TRUE(EvaluateScriptForResultBool(
+            "Object.getOwnPropertyNames(result).toString() === 'x,y,a'"));
+      });
+  // This detects a fairly subtle case: the object itself must be in the map
+  // before its properties are deserialized, so that references to it can be
+  // resolved.
+  RoundTripTest(
+      [this]() {
+        return EvaluateScriptForInput(
+            "(() => { var y = {}; y.self = y; return y; })()");
+      },
+      [this](Local<Value> value) {
+        ASSERT_TRUE(value->IsObject());
+        EXPECT_TRUE(EvaluateScriptForResultBool("result === result.self"));
+      });

[Camillo Bruni, 2016/08/16 11:26:57]

future-nit: once you can handle accessors and custom prototypes serialize a
self-modifying object (these are a the fun parts of JS!)

{a: 0, get b() { delete this.c; return 1 }, c: 2}
{a: 0, get b() { this.d = 4; return 1 }, c: 2}
{a: 0, get b() { this.__proto__ = {d:4}; return 1 }, c: 2}
{a: 0, get b() { delete this.c; this.__proto__ = {c:-2}; return 1 }, c: 2}

To continue on this, what's the take on modifying enumerability on the fly?
{ a: 0, get b() { Object.defineProperty(this, 'c', {value:-2, enumerable:
false}); return 1 }, c: 2}

[jbroman, 2016/08/16 18:16:29]

I can handle them now, aside from a bug which you caught. There's an infinite
series of possible tests that could be had here, so it's hard to judge just how
many to add. :)
This case actually caught me forgetting to check that the property was still
found (as an own property). It's a one-line check, but fixed. :)
Added variants on these tests, and a few more.
The spec says that properties are enumerated once before all [[Get]]s are done,
except for a check that the object still has that own property key before
[[Get]].

+}
+
+TEST_F(ValueSerializerTest, DecodeDictionaryObject) {
+  // Empty object.
+  DecodeTest({0xff, 0x09, 0x3f, 0x00, 0x6f, 0x7b, 0x00, 0x00},
+             [this](Local<Value> value) {
+               ASSERT_TRUE(value->IsObject());
+               EXPECT_TRUE(EvaluateScriptForResultBool(
+                   "Object.getPrototypeOf(result) === Object.prototype"));
+               EXPECT_TRUE(EvaluateScriptForResultBool(
+                   "Object.getOwnPropertyNames(result).length === 0"));
+             });
+  // String key.
+  DecodeTest(
+      {0xff, 0x09, 0x3f, 0x00, 0x6f, 0x3f, 0x01, 0x53, 0x01, 0x61, 0x3f, 0x01,
+       0x49, 0x54, 0x7b, 0x01},
+      [this](Local<Value> value) {
+        ASSERT_TRUE(value->IsObject());
+        EXPECT_TRUE(EvaluateScriptForResultBool("result.hasOwnProperty('a')"));
+        EXPECT_TRUE(EvaluateScriptForResultBool("result.a === 42"));
+        EXPECT_TRUE(EvaluateScriptForResultBool(
+            "Object.getOwnPropertyNames(result).length === 1"));
+      });
+  // Integer key (treated as a string, but may be encoded differently).
+  DecodeTest(
+      {0xff, 0x09, 0x3f, 0x00, 0x6f, 0x3f, 0x01, 0x49, 0x54, 0x3f, 0x01, 0x53,
+       0x01, 0x61, 0x7b, 0x01},
+      [this](Local<Value> value) {
+        ASSERT_TRUE(value->IsObject());
+        EXPECT_TRUE(EvaluateScriptForResultBool("result.hasOwnProperty('42')"));
+        EXPECT_TRUE(EvaluateScriptForResultBool("result[42] === 'a'"));
+        EXPECT_TRUE(EvaluateScriptForResultBool(
+            "Object.getOwnPropertyNames(result).length === 1"));
+      });
+  // Key order must be preserved.
+  DecodeTest(
+      {0xff, 0x09, 0x3f, 0x00, 0x6f, 0x3f, 0x01, 0x53, 0x01, 0x78, 0x3f, 0x01,
+       0x49, 0x02, 0x3f, 0x01, 0x53, 0x01, 0x79, 0x3f, 0x01, 0x49, 0x04, 0x3f,
+       0x01, 0x53, 0x01, 0x61, 0x3f, 0x01, 0x49, 0x06, 0x7b, 0x03},
+      [this](Local<Value> value) {
+        EXPECT_TRUE(EvaluateScriptForResultBool(
+            "Object.getOwnPropertyNames(result).toString() === 'x,y,a'"));
+      });
+  // This detects a fairly subtle case: the object itself must be in the map
+  // before its properties are deserialized, so that references to it can be
+  // resolved.
+  DecodeTest(
+      {0xff, 0x09, 0x3f, 0x00, 0x6f, 0x3f, 0x01, 0x53, 0x04, 0x73,
+       0x65, 0x6c, 0x66, 0x3f, 0x01, 0x5e, 0x00, 0x7b, 0x01, 0x00},
+      [this](Local<Value> value) {
+        ASSERT_TRUE(value->IsObject());
+        EXPECT_TRUE(EvaluateScriptForResultBool("result === result.self"));
+      });
+}
+
 }  // namespace
 }  // namespace v8