Don't send PP_Vars/V8 values with cycles across PostMessage

webkit/plugins/ppapi/v8_var_converter_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 "webkit/plugins/ppapi/v8_var_converter.h"
 
 #include <cmath>
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
@@ skipping 134 matching lines @@
     context_.Reset(isolate_, v8::Context::New(isolate_, NULL, global));
   }
   virtual void TearDown() {
     context_.Dispose();
     ASSERT_TRUE(PpapiGlobals::Get()->GetVarTracker()->GetLiveVars().empty());
     ProxyLock::Release();
   }
 
  protected:
   bool RoundTrip(const PP_Var& var, PP_Var* result) {
     V8VarConverter converter;

[dmichael (off chromium), 2013/06/18 16:27:02]

It's weird that you create this before setting up the handle scope and context
scope. I guess it's because it actually has no state that it's okay. I think it
would be better to put after the scopes.

(optional suggestion: Maybe V8VarConverter should just be a namespace)

[raymes, 2013/06/18 16:40:20]

This was a merge bug which I fixed in the latest patch. V8VarConverter is a
namespace now.

     v8::HandleScope handle_scope(isolate_);
     v8::Context::Scope context_scope(isolate_, context_);
     v8::Local<v8::Context> context =
         v8::Local<v8::Context>::New(isolate_, context_);

[dmichael (off chromium), 2013/06/18 16:27:02]

I don't have a very strong grasp on the v8 embedding API. Why do you need a new
Context?

[raymes, 2013/06/18 16:40:20]

marja@ updated this in https://chromiumcodereview.appspot.com/16690003. I
haven't looked into the details but I assume she knows what she's doing!

[dmichael (off chromium), 2013/06/18 16:51:34]

Nevermind, I was reading it incorrectly. |context| is just a local handle to
|context_|, not a _new_ context. It might be unnecessary (?) but it's certainly
not wrong.

     v8::Handle<v8::Value> v8_result;
-    if (!converter.ToV8Value(var, context, &v8_result))
+    if (!V8VarConverter::ToV8Value(var, context, &v8_result))
       return false;
     if (!Equals(var, v8_result))
       return false;
-    if (!converter.FromV8Value(v8_result, context, result))
+    if (!V8VarConverter::FromV8Value(v8_result, context, result))
       return false;
     return true;
   }
 
   // Assumes a ref for var.
   bool RoundTripAndCompare(const PP_Var& var) {
     ScopedPPVar expected(ScopedPPVar::PassRef(), var);
     PP_Var actual_var;
     if (!RoundTrip(expected.get(), &actual_var))
       return false;
@@ skipping 88 matching lines @@
   EXPECT_TRUE(RoundTripAndCompare(dictionary->GetPPVar()));
 
   // Array with dictionary.
   array->Set(index++, release_dictionary.get());
   EXPECT_TRUE(RoundTripAndCompare(array->GetPPVar()));
 
   // Array with dictionary with array.
   array2->Set(0, PP_MakeInt32(100));
   dictionary->SetWithStringKey("9", release_array2.get());
   EXPECT_TRUE(RoundTripAndCompare(array->GetPPVar()));
+}
 
-  // Array <-> dictionary cycle.
-  dictionary->SetWithStringKey("10", release_array.get());
-  PP_Var result_var;
-  EXPECT_TRUE(RoundTrip(release_dictionary.get(), &result_var));
-  ScopedPPVar result = ScopedPPVar(ScopedPPVar::PassRef(), result_var);
-  EXPECT_TRUE(TestEqual(release_dictionary.get(), result.get()));
-  // Break the cycle.
-  // TODO(raymes): We need some better machinery for releasing vars with
-  // cycles. Remove the code below once we have that.
-  dictionary->DeleteWithStringKey("10");
-  DictionaryVar* result_dictionary = DictionaryVar::FromPPVar(result.get());
-  result_dictionary->DeleteWithStringKey("10");
+TEST_F(V8VarConverterTest, Cycles) {
+  // Check that cycles aren't converted.
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
 
-  // Array with self references.
-  array->Set(index, release_array.get());
-  EXPECT_TRUE(RoundTrip(release_array.get(), &result_var));
-  result = ScopedPPVar(ScopedPPVar::PassRef(), result_var);
-  EXPECT_TRUE(TestEqual(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());
+  // Var->V8 conversion.
+  {
+    scoped_refptr<DictionaryVar> dictionary(new DictionaryVar);
+    ScopedPPVar release_dictionary(ScopedPPVar::PassRef(),
+                                   dictionary->GetPPVar());
+    scoped_refptr<ArrayVar> array(new ArrayVar);
+    ScopedPPVar release_array(ScopedPPVar::PassRef(), array->GetPPVar());
+
+    dictionary->SetWithStringKey("1", release_array.get());
+    array->Set(0, release_dictionary.get());
+
+    v8::Handle<v8::Value> v8_result;
+
+    // Array <-> dictionary cycle.
+    dictionary->SetWithStringKey("1", release_array.get());
+    ASSERT_FALSE(V8VarConverter::ToV8Value(release_dictionary.get(),
+                                           context_, &v8_result));
+    // Break the cycle.
+    // TODO(raymes): We need some better machinery for releasing vars with
+    // cycles. Remove the code below once we have that.
+    dictionary->DeleteWithStringKey("1");
+
+    // Array with self reference.
+    array->Set(0, release_array.get());
+    ASSERT_FALSE(V8VarConverter::ToV8Value(release_array.get(),
+                                           context_, &v8_result));
+    // Break the self reference.
+    array->Set(0, PP_MakeUndefined());
+  }
+
+  // V8->Var conversion.
+  {
+    v8::Handle<v8::Object> object = v8::Object::New();
+    v8::Handle<v8::Array> array = v8::Array::New();
+
+    PP_Var var_result;
+
+    // Array <-> dictionary cycle.
+    std::string key = "1";
+    object->Set(v8::String::New(key.c_str(), key.length()), array);
+    array->Set(0, object);
+
+    ASSERT_FALSE(V8VarConverter::FromV8Value(object, context_, &var_result));
+
+    // Array with self reference.
+    array->Set(0, array);
+    ASSERT_FALSE(V8VarConverter::FromV8Value(array, context_, &var_result));
+  }
 }
 
 TEST_F(V8VarConverterTest, StrangeDictionaryKeyTest) {
   {
     // Test keys with '.'.
     scoped_refptr<DictionaryVar> dictionary(new DictionaryVar);
     dictionary->SetWithStringKey(".", PP_MakeUndefined());
     dictionary->SetWithStringKey("x.y", PP_MakeUndefined());
     EXPECT_TRUE(RoundTripAndCompare(dictionary->GetPPVar()));
   }
@@ skipping 11 matching lines @@
           "false: 'qux',"
           "null: 'quux',"
           "undefined: 'oops'"
         "};"
         "})();";
 
     v8::Handle<v8::Script> script(v8::Script::New(v8::String::New(source)));
     v8::Handle<v8::Object> object = script->Run().As<v8::Object>();
     ASSERT_FALSE(object.IsEmpty());
 
-    V8VarConverter converter;
     PP_Var actual;
-    ASSERT_TRUE(converter.FromV8Value(
-        object, v8::Local<v8::Context>::New(isolate_, context_), &actual));
+    ASSERT_TRUE(V8VarConverter::FromV8Value(object,
+        v8::Local<v8::Context>::New(isolate_, context_), &actual));
     ScopedPPVar release_actual(ScopedPPVar::PassRef(), actual);
 
     scoped_refptr<DictionaryVar> expected(new DictionaryVar);
     ScopedPPVar foo(ScopedPPVar::PassRef(), StringVar::StringToPPVar("foo"));
     expected->SetWithStringKey("1", foo.get());
     ScopedPPVar bar(ScopedPPVar::PassRef(), StringVar::StringToPPVar("bar"));
     expected->SetWithStringKey("2", bar.get());
     ScopedPPVar baz(ScopedPPVar::PassRef(), StringVar::StringToPPVar("baz"));
     expected->SetWithStringKey("true", baz.get());
     ScopedPPVar qux(ScopedPPVar::PassRef(), StringVar::StringToPPVar("qux"));
     expected->SetWithStringKey("false", qux.get());
     ScopedPPVar quux(ScopedPPVar::PassRef(), StringVar::StringToPPVar("quux"));
     expected->SetWithStringKey("null", quux.get());
     ScopedPPVar oops(ScopedPPVar::PassRef(), StringVar::StringToPPVar("oops"));
     expected->SetWithStringKey("undefined", oops.get());
     ScopedPPVar release_expected(
         ScopedPPVar::PassRef(), expected->GetPPVar());
 
     ASSERT_TRUE(TestEqual(release_expected.get(), release_actual.get()));
   }
 }
 
 }  // namespace ppapi
 }  // namespace webkit