Introduce RawVarData and associated classes for serializing PP_Vars

ppapi/proxy/raw_var_data_unittest.cc

+// Copyright (c) 2012 The Chromium 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 "ppapi/proxy/raw_var_data.h"
+
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "ppapi/c/pp_bool.h"
+#include "ppapi/c/pp_var.h"
+#include "ppapi/shared_impl/array_var.h"
+#include "ppapi/shared_impl/dictionary_var.h"
+#include "ppapi/shared_impl/ppapi_globals.h"
+#include "ppapi/shared_impl/proxy_lock.h"
+#include "ppapi/shared_impl/scoped_pp_var.h"
+#include "ppapi/shared_impl/test_globals.h"
+#include "ppapi/shared_impl/var.h"
+#include "ppapi/shared_impl/var_tracker.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ppapi {
+namespace proxy {
+
+namespace {
+
+class RawVarDataTest : public testing::Test {
+ public:
+  RawVarDataTest() {}
+  ~RawVarDataTest() {}
+
+  // testing::Test implementation.
+  virtual void SetUp() {
+    ProxyLock::Acquire();
+  }
+  virtual void TearDown() {
+    ASSERT_TRUE(PpapiGlobals::Get()->GetVarTracker()->GetLiveVars().empty());
+    ProxyLock::Release();
+  }
+
+ private:
+  TestGlobals globals_;
+};
+
+bool Equals(const PP_Var& expected,
+            const PP_Var& actual,
+            base::hash_map<int64_t, int64_t>* visited_map) {

[dmichael (off chromium), 2013/04/16 16:36:53]

May be worth a comment...  it seems visited_map is not just a way to tell what's
been visited, but also maps the source Var id to the target Var id after
serialization/deserialization. (A.k.a. maps the id of a Var in |expected| to the
id of its clone in |actual|). It took me a minute to figure out what was going
on.

[raymes1, 2013/04/16 18:27:19]

Done.

+  if (expected.type != actual.type)
+    return false;
+  if (VarTracker::IsVarTypeRefcounted(expected.type)) {
+    base::hash_map<int64_t, int64_t>::iterator it =
+        visited_map->find(expected.value.as_id);
+    if (it != visited_map->end())
+      return it->second == actual.value.as_id;
+    else
+      (*visited_map)[expected.value.as_id] = actual.value.as_id;
+  }
+  switch (expected.type) {
+    case PP_VARTYPE_UNDEFINED:
+      return true;
+    case PP_VARTYPE_NULL:
+      return true;
+    case PP_VARTYPE_BOOL:
+      return expected.value.as_bool == actual.value.as_bool;
+    case PP_VARTYPE_INT32:
+      return expected.value.as_int == actual.value.as_int;
+    case PP_VARTYPE_DOUBLE:
+      return expected.value.as_double == actual.value.as_double;
+    case PP_VARTYPE_OBJECT:
+      return expected.value.as_id == actual.value.as_id;

[dmichael (off chromium), 2013/04/16 16:36:53]

This could be a DCHECK instead, right?

[raymes1, 2013/04/16 18:27:19]

Not sure exactly what you mean? In the old code, the SerializedVar::Inner actual
just transmitted the var ID. So I have kept the same behavior. This check
verifies that case is handled correctly.

[dmichael (off chromium), 2013/04/16 18:41:20]

You're right, nevermind. Was confused because we don't support
PostMessage/HandleMessage for these.

+    case PP_VARTYPE_STRING: {
+      StringVar* expected_var = StringVar::FromPPVar(expected);
+      StringVar* actual_var = StringVar::FromPPVar(actual);
+      if (!expected_var || !actual_var)
+        return expected_var == actual_var;

[dmichael (off chromium), 2013/04/16 16:36:53]

Since we want to bail for invalid Vars, should this instead to a DCHECK or
something? (similar for other refcounted types)

[raymes1, 2013/04/16 18:27:19]

Good idea this was a remnant from when I handled those cases.

+      return expected_var->value() == expected_var->value();
+    }
+    case PP_VARTYPE_ARRAY_BUFFER: {
+      ArrayBufferVar* expected_var = ArrayBufferVar::FromPPVar(expected);
+      ArrayBufferVar* actual_var = ArrayBufferVar::FromPPVar(actual);
+      if (!expected_var || !actual_var)
+        return expected_var == actual_var;
+      std::string expected_data(static_cast<const char*>(expected_var->Map()),
+                                expected_var->ByteLength());
+      std::string actual_data(static_cast<const char*>(actual_var->Map()),
+                              actual_var->ByteLength());
+      return expected_data == actual_data;

[dmichael (off chromium), 2013/04/16 16:36:53]

Optional: It feels a little odd to copy them just to compare. You could do
something like:
if (expected_var->ByteLength() != actual_var->ByteLength())
  return false;
return memcmp(expected_var->Map(), actual_var->Map(),
              expected_var->ByteLength()) == 0);

Granted, it's a test, so not that important to be optimized.

[raymes1, 2013/04/16 18:27:19]

haha I avoid C memory functions like the plague and since it was a test I did
the inefficient thing. However, since you provided the code, done :)

[dmichael (off chromium), 2013/04/16 18:41:20]

I almost suggested std::equal, but you would have to cast the result of Map,
which would be kind of annoying. Do whichever you like :-)

+    }
+    case PP_VARTYPE_ARRAY: {
+      ArrayVar* expected_var = ArrayVar::FromPPVar(expected);
+      ArrayVar* actual_var = ArrayVar::FromPPVar(actual);
+      if (!expected_var || !actual_var)
+        return expected_var == actual_var;
+      if (expected_var->elements().size() != actual_var->elements().size())
+        return false;
+      for (size_t i = 0; i < expected_var->elements().size(); ++i) {
+        if (!Equals(expected_var->elements()[i].get(),
+                    actual_var->elements()[i].get(),
+                    visited_map)) {
+          return false;
+        }
+      }
+      return true;
+    }
+    case PP_VARTYPE_DICTIONARY: {
+      DictionaryVar* expected_var = DictionaryVar::FromPPVar(expected);
+      DictionaryVar* actual_var = DictionaryVar::FromPPVar(actual);
+      if (!expected_var || !actual_var)
+        return expected_var == actual_var;
+      if (expected_var->key_value_map().size() !=
+          actual_var->key_value_map().size())
+        return false;
+      DictionaryVar::KeyValueMap::const_iterator expected_iter =
+          expected_var->key_value_map().begin();
+      DictionaryVar::KeyValueMap::const_iterator actual_iter =
+          actual_var->key_value_map().begin();
+      for ( ; expected_iter != expected_var->key_value_map().end() &&
+               actual_iter != actual_var->key_value_map().end();

[dmichael (off chromium), 2013/04/16 16:36:53]

nit/suggestion: you don't really need to bounds check both iterators, since you
know the maps are equal size. That might save you from the kind of weird
formatting here.

[raymes1, 2013/04/16 18:27:19]

Done.

+           ++expected_iter, ++actual_iter) {
+           if (expected_iter->first != actual_iter->first)
+             return false;
+        if (!Equals(expected_iter->second.get(),
+                    actual_iter->second.get(),
+                    visited_map)) {
+          return false;
+        }
+      }
+      return true;
+    }
+  }
+  NOTREACHED();
+  return false;
+}

[dmichael (off chromium), 2013/04/16 16:36:53]

Optional suggestion: You might want to have logging for places where you detect
inequality, to make debugging easier. Or not...  somebody debugging a problem
can do that later.

[raymes1, 2013/04/16 18:27:19]

Ok - it's a good suggestion but I just added a TODO for now.

+
+bool Equals(const PP_Var& expected,
+            const PP_Var& actual) {
+  base::hash_map<int64_t, int64_t> visited_map;
+  return Equals(expected, actual, &visited_map);
+}
+
+PP_Var WriteAndRead(const PP_Var& var) {
+  PP_Instance dummy_instance = 1234;
+  scoped_ptr<RawVarDataGraph> expected_data(RawVarDataGraph::Create(
+      var, dummy_instance));
+  IPC::Message m;
+  expected_data->Write(&m);
+  PickleIterator iter(m);
+  scoped_ptr<RawVarDataGraph> actual_data(RawVarDataGraph::Read(&m, &iter));
+  return actual_data->CreatePPVar(dummy_instance);
+}
+
+// Assumes a ref for var.
+bool WriteReadAndCompare(const PP_Var& var) {
+  ScopedPPVar expected(ScopedPPVar::PassRef(), var);
+  ScopedPPVar actual(ScopedPPVar::PassRef(), WriteAndRead(expected.get()));
+  return Equals(expected.get(), actual.get());
+}
+
+}  // namespace
+
+TEST_F(RawVarDataTest, SimpleTest) {
+  EXPECT_TRUE(WriteReadAndCompare(PP_MakeUndefined()));
+  EXPECT_TRUE(WriteReadAndCompare(PP_MakeNull()));
+  EXPECT_TRUE(WriteReadAndCompare(PP_MakeInt32(100)));
+  EXPECT_TRUE(WriteReadAndCompare(PP_MakeBool(PP_TRUE)));
+  EXPECT_TRUE(WriteReadAndCompare(PP_MakeDouble(53.75)));
+  PP_Var object;
+  object.type = PP_VARTYPE_OBJECT;
+  object.value.as_id = 10;
+  EXPECT_TRUE(WriteReadAndCompare(object));
+}
+
+TEST_F(RawVarDataTest, StringTest) {
+  EXPECT_TRUE(WriteReadAndCompare(StringVar::StringToPPVar("")));
+  EXPECT_TRUE(WriteReadAndCompare(StringVar::StringToPPVar("hello world!")));
+}
+
+TEST_F(RawVarDataTest, ArrayBufferTest) {
+  std::string data = "hello world!";
+  PP_Var var = PpapiGlobals::Get()->GetVarTracker()->MakeArrayBufferPPVar(
+      data.size(), data.data());
+  EXPECT_TRUE(WriteReadAndCompare(var));
+  var = PpapiGlobals::Get()->GetVarTracker()->MakeArrayBufferPPVar(
+      0, static_cast<void*>(NULL));
+  EXPECT_TRUE(WriteReadAndCompare(var));
+  // TODO(raymes): add tests for shmem type array buffers.
+}
+
+TEST_F(RawVarDataTest, DictionaryArrayTest) {
+  // Empty array.
+  scoped_refptr<ArrayVar> array(new ArrayVar);
+  ScopedPPVar release_array(ScopedPPVar::PassRef(), array->GetPPVar());
+  EXPECT_TRUE(WriteReadAndCompare(array->GetPPVar()));
+
+  size_t index = 0;
+
+  // Array with primitives.
+  array->Set(index++, PP_MakeUndefined());
+  array->Set(index++, PP_MakeNull());
+  array->Set(index++, PP_MakeInt32(100));
+  array->Set(index++, PP_MakeBool(PP_FALSE));
+  array->Set(index++, PP_MakeDouble(0.123));
+  EXPECT_TRUE(WriteReadAndCompare(array->GetPPVar()));
+
+  // Empty dictionary.
+  scoped_refptr<DictionaryVar> dictionary(new DictionaryVar);
+  ScopedPPVar release_dictionary(ScopedPPVar::PassRef(),
+                                 dictionary->GetPPVar());
+  EXPECT_TRUE(WriteReadAndCompare(dictionary->GetPPVar()));
+
+  // Dictionary with primitives.
+  ScopedPPVar key1(ScopedPPVar::PassRef(), StringVar::StringToPPVar("1"));
+  dictionary->Set(key1.get(), PP_MakeUndefined());
+  ScopedPPVar key2(ScopedPPVar::PassRef(), StringVar::StringToPPVar("2"));
+  dictionary->Set(key2.get(), PP_MakeNull());
+  ScopedPPVar key3(ScopedPPVar::PassRef(), StringVar::StringToPPVar("3"));
+  dictionary->Set(key3.get(), PP_MakeInt32(-100));
+  ScopedPPVar key4(ScopedPPVar::PassRef(), StringVar::StringToPPVar("4"));
+  dictionary->Set(key4.get(), PP_MakeBool(PP_TRUE));
+  ScopedPPVar key5(ScopedPPVar::PassRef(), StringVar::StringToPPVar("5"));
+  dictionary->Set(key5.get(), PP_MakeDouble(-103.52));
+  EXPECT_TRUE(WriteReadAndCompare(dictionary->GetPPVar()));
+
+  // Array with dictionary.
+  array->Set(index++, release_dictionary.get());
+  EXPECT_TRUE(WriteReadAndCompare(array->GetPPVar()));
+
+  // Array with dictionary with array.
+  scoped_refptr<ArrayVar> array2(new ArrayVar);
+  ScopedPPVar release_array2(ScopedPPVar::PassRef(), array2->GetPPVar());
+  array2->Set(0, PP_MakeInt32(100));
+  ScopedPPVar key6(ScopedPPVar::PassRef(), StringVar::StringToPPVar("6"));
+  dictionary->Set(key6.get(), release_array2.get());
+  EXPECT_TRUE(WriteReadAndCompare(array->GetPPVar()));
+
+  // Array <-> dictionary cycle.
+  ScopedPPVar key7(ScopedPPVar::PassRef(), StringVar::StringToPPVar("7"));
+  dictionary->Set(key7.get(), release_array.get());
+  ScopedPPVar result = ScopedPPVar(ScopedPPVar::PassRef(),
+                                   WriteAndRead(release_dictionary.get()));
+  EXPECT_TRUE(Equals(release_dictionary.get(), result.get()));
+  // Break the cycle.

[dmichael (off chromium), 2013/04/16 16:36:53]

This is because we'll leak it otherwise?

I feel like we should consider fixing that. IMHO, we already need to improve our
VarTracker release code. I'm fairly sure recursively deleting a deep Var is
going to cause stack overflows right now. Probably would only take a few hundred
levels deep.

But that should be a separate CL, in any case. Maybe a TODO here would suffice.

[raymes1, 2013/04/16 18:27:19]

Yes we leak otherwise. I agree. We need to provide something better for handling
cycles. Added a TODO.

+  dictionary->Delete(key7.get());
+  DictionaryVar* result_dictionary = DictionaryVar::FromPPVar(result.get());
+  result_dictionary->Delete(key7.get());
+
+  // Array with self references.
+  array->Set(index, release_array.get());
+  result = ScopedPPVar(ScopedPPVar::PassRef(),
+                       WriteAndRead(release_array.get()));
+  EXPECT_TRUE(Equals(release_array.get(), result.get()));
+  // Break the self reference.
+  array->Set(index, PP_MakeUndefined());
+  ArrayVar* result_array = ArrayVar::FromPPVar(result.get());
+  result_array->Set(index, PP_MakeUndefined());
+}

[dmichael (off chromium), 2013/04/16 16:36:53]

How about a test where one direct child references another child, for both
Dictionary and Array?
E.g.:
array->Set(0, release_array2.get());
array->Set(1, some_string_var.get());
array2->Set(0, some_String_var.get());

So not a cycle, but more like a diamond:
Array1 ---------\
  |             |
  v             v
Array2 ----> String
 I'm pretty sure your code handles it right, but some implementations would
handle cycles but not this.

[raymes1, 2013/04/16 18:27:19]

Good idea. I also forgot to add a test for the simple case of referencing (for
example) the same string twice in an array.

+
+}  // namespace proxy
+}  // namespace ppapi