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

This prevents PP_Vars/V8 values with cycles being transmitted across PostMessage. An undefined value will be sent instead and an error will be logged to the console.

BUG=236958

Review URL: https://chromiumcodereview.appspot.com/16140011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@207040 0039d316-1c4b-4281-b951-d872f2087c98

18 files changed, 396 insertions, 184 deletions

diff --git a/ppapi/api/ppb_messaging.idl b/ppapi/api/ppb_messaging.idl
index 647f7f6..0f9a3aa 100644
--- a/ppapi/api/ppb_messaging.idl
+++ b/ppapi/api/ppb_messaging.idl
@@ -36,8 +36,8 @@ interface PPB_Messaging {
    * JavaScript.
    *
    * When passing array or dictionary <code>PP_Var</code>s, the entire reference
-   * graph will be converted and transferred, including reference cycles if they
-   * exist.
+   * graph will be converted and transferred. If the reference graph has cycles,
+   * the message will not be sent and an error will be logged to the console.
    *
    * Listeners for message events in JavaScript code will receive an object
    * conforming to the HTML 5 <code>MessageEvent</code> interface.
diff --git a/ppapi/api/ppp_messaging.idl b/ppapi/api/ppp_messaging.idl
index 150f771..83c2010 100644
--- a/ppapi/api/ppp_messaging.idl
+++ b/ppapi/api/ppp_messaging.idl
@@ -36,10 +36,9 @@ interface PPP_Messaging {
    *
    * When converting JavaScript arrays, any object properties whose name
    * is not an array index are ignored. When passing arrays and objects, the
-   * entire reference graph will be converted and transferred, including
-   * reference cycles if they exist. Since <code>PP_Var</code>s are ref-counted,
-   * the author of the plugin must take care if they expect to receive vars with
-   * cycles. Cycles must be manually broken to correctly release the vars.
+   * entire reference graph will be converted and transferred. If the reference
+   * graph has cycles, the message will not be sent and an error will be logged
+   * to the console.
    *
    * The following JavaScript code invokes <code>HandleMessage</code>, passing
    * the module instance on which it was invoked, with <code>message</code>
diff --git a/ppapi/c/ppb_messaging.h b/ppapi/c/ppb_messaging.h
index 404a11c..06e31205 100644
--- a/ppapi/c/ppb_messaging.h
+++ b/ppapi/c/ppb_messaging.h
@@ -3,7 +3,7 @@
  * found in the LICENSE file.
  */
 
-/* From ppb_messaging.idl modified Mon May 20 15:31:07 2013. */
+/* From ppb_messaging.idl modified Wed Jun  5 10:32:59 2013. */
 
 #ifndef PPAPI_C_PPB_MESSAGING_H_
 #define PPAPI_C_PPB_MESSAGING_H_
@@ -50,8 +50,8 @@ struct PPB_Messaging_1_0 {
    * JavaScript.
    *
    * When passing array or dictionary <code>PP_Var</code>s, the entire reference
-   * graph will be converted and transferred, including reference cycles if they
-   * exist.
+   * graph will be converted and transferred. If the reference graph has cycles,
+   * the message will not be sent and an error will be logged to the console.
    *
    * Listeners for message events in JavaScript code will receive an object
    * conforming to the HTML 5 <code>MessageEvent</code> interface.
diff --git a/ppapi/c/ppp_messaging.h b/ppapi/c/ppp_messaging.h
index b2e1f3c..28d40d6 100644
--- a/ppapi/c/ppp_messaging.h
+++ b/ppapi/c/ppp_messaging.h
@@ -3,7 +3,7 @@
  * found in the LICENSE file.
  */
 
-/* From ppp_messaging.idl modified Tue May 21 09:01:17 2013. */
+/* From ppp_messaging.idl modified Wed Jun  5 10:32:43 2013. */
 
 #ifndef PPAPI_C_PPP_MESSAGING_H_
 #define PPAPI_C_PPP_MESSAGING_H_
@@ -52,10 +52,9 @@ struct PPP_Messaging_1_0 {
    *
    * When converting JavaScript arrays, any object properties whose name
    * is not an array index are ignored. When passing arrays and objects, the
-   * entire reference graph will be converted and transferred, including
-   * reference cycles if they exist. Since <code>PP_Var</code>s are ref-counted,
-   * the author of the plugin must take care if they expect to receive vars with
-   * cycles. Cycles must be manually broken to correctly release the vars.
+   * entire reference graph will be converted and transferred. If the reference
+   * graph has cycles, the message will not be sent and an error will be logged
+   * to the console.
    *
    * The following JavaScript code invokes <code>HandleMessage</code>, passing
    * the module instance on which it was invoked, with <code>message</code>
diff --git a/ppapi/cpp/instance.h b/ppapi/cpp/instance.h
index 48bca90..6bf4e83 100644
--- a/ppapi/cpp/instance.h
+++ b/ppapi/cpp/instance.h
@@ -244,6 +244,12 @@ class Instance {
   /// JavaScript execution will not be blocked while HandleMessage() is
   /// processing the message.
   ///
+  /// When converting JavaScript arrays, any object properties whose name
+  /// is not an array index are ignored. When passing arrays and objects, the
+  /// entire reference graph will be converted and transferred. If the reference
+  /// graph has cycles, the message will not be sent and an error will be logged
+  /// to the console.
+  ///
   /// <strong>Example:</strong>
   ///
   /// The following JavaScript code invokes <code>HandleMessage</code>, passing
@@ -264,9 +270,10 @@ class Instance {
   ///
   /// Refer to PostMessage() for sending messages to JavaScript.
   ///
-  /// @param[in] message A <code>Var</code> containing the data sent from
-  /// JavaScript. Message can have an int32_t, double, bool, or string value
-  /// (objects are not supported).
+  /// @param[in] message A <code>Var</code> which has been converted from a
+  /// JavaScript value. JavaScript array/object types are supported from Chrome
+  /// M29 onward. All JavaScript values are copied when passing them to the
+  /// plugin.
   virtual void HandleMessage(const Var& message);
 
   /// @}
@@ -453,6 +460,11 @@ class Instance {
   ///
   /// The browser will pop-up an alert saying "Hello world!"
   ///
+  /// When passing array or dictionary <code>PP_Var</code>s, the entire
+  /// reference graph will be converted and transferred. If the reference graph
+  /// has cycles, the message will not be sent and an error will be logged to
+  /// the console.
+  ///
   /// Listeners for message events in JavaScript code will receive an object
   /// conforming to the HTML 5 <code>MessageEvent</code> interface.
   /// Specifically, the value of message will be contained as a property called
@@ -466,9 +478,9 @@ class Instance {
   /// Refer to HandleMessage() for receiving events from JavaScript.
   ///
   /// @param[in] message A <code>Var</code> containing the data to be sent to
-  /// JavaScript. Message can have a numeric, boolean, or string value; arrays
-  /// and dictionaries are not yet supported. Ref-counted var types are copied,
-  /// and are therefore not shared between the instance and the browser.
+  /// JavaScript. Message can have a numeric, boolean, or string value.
+  /// Array/Dictionary types are supported from Chrome M29 onward.
+  /// All var types are copied when passing them to JavaScript.
   void PostMessage(const Var& message);
 
   /// @}
diff --git a/ppapi/proxy/handle_converter.cc b/ppapi/proxy/handle_converter.cc
index b77ee49..d2fc4a9 100644
--- a/ppapi/proxy/handle_converter.cc
+++ b/ppapi/proxy/handle_converter.cc
@@ -54,18 +54,14 @@ void ConvertHandlesInParam(const ppapi::proxy::SerializedVar& var,
                            Handles* handles,
                            IPC::Message* msg,
                            int* handle_index) {
-  if (!var.raw_var_data())
-    return;
-
-  std::vector<ppapi::proxy::SerializedHandle*> var_handles =
-      var.raw_var_data()->GetHandles();
+  std::vector<ppapi::proxy::SerializedHandle*> var_handles = var.GetHandles();
   if (var_handles.empty())
     return;
 
   for (size_t i = 0; i < var_handles.size(); ++i)
     handles->push_back(*var_handles[i]);
   if (msg)
-    var.raw_var_data()->Write(msg, base::Bind(&HandleWriter, handle_index));
+    var.WriteDataToMessage(msg, base::Bind(&HandleWriter, handle_index));
 }
 
 // For PpapiMsg_ResourceReply and the reply to PpapiHostMsg_ResourceSyncCall,
diff --git a/ppapi/proxy/ppb_instance_proxy.cc b/ppapi/proxy/ppb_instance_proxy.cc
index 4009a19..e3948f06a 100644
--- a/ppapi/proxy/ppb_instance_proxy.cc
+++ b/ppapi/proxy/ppb_instance_proxy.cc
@@ -56,6 +56,10 @@ namespace proxy {
 
 namespace {
 
+const char kSerializationError[] = "Failed to convert a PostMessage "
+    "argument from a PP_Var to a Javascript value. It may have cycles or be of "
+    "an unsupported type.";
+
 InterfaceProxy* CreateInstanceProxy(Dispatcher* dispatcher) {
   return new PPB_Instance_Proxy(dispatcher);
 }
@@ -933,6 +937,12 @@ void PPB_Instance_Proxy::OnHostMsgPostMessage(
     PP_Instance instance,
     SerializedVarReceiveInput message) {
   EnterInstanceNoLock enter(instance);
+  if (!message.is_valid_var()) {
+    PpapiGlobals::Get()->LogWithSource(
+        instance, PP_LOGLEVEL_ERROR, std::string(), kSerializationError);
+    return;
+  }
+
   if (enter.succeeded())
     enter.functions()->PostMessage(instance,
                                    message.GetForInstance(dispatcher(),
diff --git a/ppapi/proxy/raw_var_data.cc b/ppapi/proxy/raw_var_data.cc
index 4de51ae..a550c3f 100644
--- a/ppapi/proxy/raw_var_data.cc
+++ b/ppapi/proxy/raw_var_data.cc
@@ -31,25 +31,27 @@ static const uint32 kMinimumArrayBufferSizeForShmem = 256 * 1024;
 static uint32 g_minimum_array_buffer_size_for_shmem =
     kMinimumArrayBufferSizeForShmem;
 
-void DefaultHandleWriter(IPC::Message* m, const SerializedHandle& handle) {
-  IPC::ParamTraits<SerializedHandle>::Write(m, handle);
-}
+struct StackEntry {
+  StackEntry(PP_Var v, size_t i) : var(v), data_index(i) {}
+  PP_Var var;
+  size_t data_index;
+};
 
 // For a given PP_Var, returns the RawVarData associated with it, or creates a
 // new one if there is no existing one. The data is appended to |data| if it
 // is newly created. The index into |data| pointing to the result is returned.
-// |id_map| keeps track of RawVarDatas that have already been created.
+// |visited_map| keeps track of RawVarDatas that have already been created.
 size_t GetOrCreateRawVarData(const PP_Var& var,
-                             base::hash_map<int64_t, size_t>* id_map,
+                             base::hash_map<int64_t, size_t>* visited_map,
                              ScopedVector<RawVarData>* data) {
   if (VarTracker::IsVarTypeRefcounted(var.type)) {
-    base::hash_map<int64_t, size_t>::iterator it = id_map->find(
+    base::hash_map<int64_t, size_t>::iterator it = visited_map->find(
         var.value.as_id);
-    if (it != id_map->end()) {
+    if (it != visited_map->end()) {
       return it->second;
     } else {
       data->push_back(RawVarData::Create(var.type));
-      (*id_map)[var.value.as_id] = data->size() - 1;
+      (*visited_map)[var.value.as_id] = data->size() - 1;
     }
   } else {
     data->push_back(RawVarData::Create(var.type));
@@ -57,6 +59,10 @@ size_t GetOrCreateRawVarData(const PP_Var& var,
   return data->size() - 1;
 }
 
+bool CanHaveChildren(PP_Var var) {
+  return var.type == PP_VARTYPE_ARRAY || var.type == PP_VARTYPE_DICTIONARY;
+}
+
 }  // namespace
 
 // RawVarDataGraph ------------------------------------------------------------
@@ -66,27 +72,40 @@ RawVarDataGraph::RawVarDataGraph() {
 RawVarDataGraph::~RawVarDataGraph() {
 }
 
+// This function uses a stack-based DFS search to traverse the var graph. Each
+// iteration, the top node on the stack examined. If the node has not been
+// visited yet (i.e. !initialized()) then it is added to the list of
+// |parent_ids| which contains all of the nodes on the path from the start node
+// to the current node. Each of that nodes children are examined. If they appear
+// in the list of |parent_ids| it means we have a cycle and we return NULL.
+// Otherwise, if they haven't been visited yet we add them to the stack, If the
+// node at the top of the stack has already been visited, then we pop it off the
+// stack and erase it from |parent_ids|.
 // static
 scoped_ptr<RawVarDataGraph> RawVarDataGraph::Create(const PP_Var& var,
                                                     PP_Instance instance) {
   scoped_ptr<RawVarDataGraph> graph(new RawVarDataGraph);
   // Map of |var.value.as_id| to a RawVarData index in RawVarDataGraph.
-  base::hash_map<int64_t, size_t> id_map;
+  base::hash_map<int64_t, size_t> visited_map;
+  base::hash_set<int64_t> parent_ids;
 
-  std::stack<std::pair<PP_Var, size_t> > stack;
-  stack.push(make_pair(var, GetOrCreateRawVarData(var, &id_map,
-                                                  &graph->data_)));
+  std::stack<StackEntry> stack;
+  stack.push(StackEntry(var, GetOrCreateRawVarData(var, &visited_map,
+                                                   &graph->data_)));
 
-  // Traverse the PP_Var graph with DFS.
   while (!stack.empty()) {
-    PP_Var current_var = stack.top().first;
-    RawVarData* current_var_data = graph->data_[stack.top().second];
-    stack.pop();
+    PP_Var current_var = stack.top().var;
+    RawVarData* current_var_data = graph->data_[stack.top().data_index];
 
-    // If the node is initialized, it means we have visited it.
-    if (current_var_data->initialized())
+    if (current_var_data->initialized()) {
+      stack.pop();
+      if (CanHaveChildren(current_var))
+        parent_ids.erase(current_var.value.as_id);
       continue;
+    }
 
+    if (CanHaveChildren(current_var))
+      parent_ids.insert(current_var.value.as_id);
     if (!current_var_data->Init(current_var, instance)) {
       NOTREACHED();
       return scoped_ptr<RawVarDataGraph>();
@@ -103,10 +122,16 @@ scoped_ptr<RawVarDataGraph> RawVarDataGraph::Create(const PP_Var& var,
                array_var->elements().begin();
            iter != array_var->elements().end();
            ++iter) {
-        size_t child_id = GetOrCreateRawVarData(iter->get(), &id_map,
+        const PP_Var& child = iter->get();
+        // If a child of this node is already in parent_ids, we have a cycle so
+        // we just return null.
+        if (CanHaveChildren(child) && parent_ids.count(child.value.as_id) != 0)
+          return scoped_ptr<RawVarDataGraph>();
+        size_t child_id = GetOrCreateRawVarData(child, &visited_map,
                                                 &graph->data_);
         static_cast<ArrayRawVarData*>(current_var_data)->AddChild(child_id);
-        stack.push(make_pair(iter->get(), child_id));
+        if (!graph->data_[child_id]->initialized())
+          stack.push(StackEntry(child, child_id));
       }
     } else if (current_var.type == PP_VARTYPE_DICTIONARY) {
       DictionaryVar* dict_var = DictionaryVar::FromPPVar(current_var);
@@ -118,11 +143,15 @@ scoped_ptr<RawVarDataGraph> RawVarDataGraph::Create(const PP_Var& var,
                dict_var->key_value_map().begin();
            iter != dict_var->key_value_map().end();
            ++iter) {
-        size_t child_id = GetOrCreateRawVarData(iter->second.get(), &id_map,
+        const PP_Var& child = iter->second.get();
+        if (CanHaveChildren(child) && parent_ids.count(child.value.as_id) != 0)
+          return scoped_ptr<RawVarDataGraph>();
+        size_t child_id = GetOrCreateRawVarData(child, &visited_map,
                                                 &graph->data_);
         static_cast<DictionaryRawVarData*>(
             current_var_data)->AddChild(iter->first, child_id);
-        stack.push(make_pair(iter->second.get(), child_id));
+        if (!graph->data_[child_id]->initialized())
+          stack.push(StackEntry(child, child_id));
       }
     }
   }
@@ -153,10 +182,6 @@ void RawVarDataGraph::Write(IPC::Message* m,
   }
 }
 
-void RawVarDataGraph::Write(IPC::Message* m) {
-  Write(m, base::Bind(&DefaultHandleWriter));
-}
-
 // static
 scoped_ptr<RawVarDataGraph> RawVarDataGraph::Read(const IPC::Message* m,
                                                   PickleIterator* iter) {
diff --git a/ppapi/proxy/raw_var_data.h b/ppapi/proxy/raw_var_data.h
index 1e8746f..c7981d3 100644
--- a/ppapi/proxy/raw_var_data.h
+++ b/ppapi/proxy/raw_var_data.h
@@ -65,8 +65,6 @@ class PPAPI_PROXY_EXPORT RawVarDataGraph {
 
   // Write the graph to a message using the given HandleWriter.
   void Write(IPC::Message* m, const HandleWriter& handle_writer);
-  // Write the graph to a message using the default handle writer.
-  void Write(IPC::Message* m);
 
   // Create a RawVarDataGraph from the given message.
   static scoped_ptr<RawVarDataGraph> Read(const IPC::Message* m,
diff --git a/ppapi/proxy/raw_var_data_unittest.cc b/ppapi/proxy/raw_var_data_unittest.cc
index 2b134af..2ee6914 100644
--- a/ppapi/proxy/raw_var_data_unittest.cc
+++ b/ppapi/proxy/raw_var_data_unittest.cc
@@ -26,6 +26,10 @@ namespace proxy {
 
 namespace {
 
+void DefaultHandleWriter(IPC::Message* m, const SerializedHandle& handle) {
+  IPC::ParamTraits<SerializedHandle>::Write(m, handle);
+}
+
 class RawVarDataTest : public testing::Test {
  public:
   RawVarDataTest() {}
@@ -44,21 +48,28 @@ class RawVarDataTest : public testing::Test {
   TestGlobals globals_;
 };
 
-PP_Var WriteAndRead(const PP_Var& var) {
+bool WriteAndRead(const PP_Var& var, PP_Var* result) {
   PP_Instance dummy_instance = 1234;
   scoped_ptr<RawVarDataGraph> expected_data(RawVarDataGraph::Create(
       var, dummy_instance));
+  if (!expected_data)
+    return false;
   IPC::Message m;
-  expected_data->Write(&m);
+  expected_data->Write(&m, base::Bind(&DefaultHandleWriter));
   PickleIterator iter(m);
   scoped_ptr<RawVarDataGraph> actual_data(RawVarDataGraph::Read(&m, &iter));
-  return actual_data->CreatePPVar(dummy_instance);
+  *result = actual_data->CreatePPVar(dummy_instance);
+  return true;
 }
 
 // Assumes a ref for var.
 bool WriteReadAndCompare(const PP_Var& var) {
   ScopedPPVar expected(ScopedPPVar::PassRef(), var);
-  ScopedPPVar actual(ScopedPPVar::PassRef(), WriteAndRead(expected.get()));
+  PP_Var result;
+  bool success = WriteAndRead(expected.get(), &result);
+  if (!success)
+    return false;
+  ScopedPPVar actual(ScopedPPVar::PassRef(), result);
   return TestEqual(expected.get(), actual.get());
 }
 
@@ -160,25 +171,18 @@ TEST_F(RawVarDataTest, DictionaryArrayTest) {
 
   // Array <-> dictionary cycle.
   dictionary->SetWithStringKey("10", release_array.get());
-  ScopedPPVar result = ScopedPPVar(ScopedPPVar::PassRef(),
-                                   WriteAndRead(release_dictionary.get()));
-  EXPECT_TRUE(TestEqual(release_dictionary.get(), result.get()));
+  PP_Var result;
+  ASSERT_FALSE(WriteAndRead(release_dictionary.get(), &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("10");
-  DictionaryVar* result_dictionary = DictionaryVar::FromPPVar(result.get());
-  result_dictionary->DeleteWithStringKey("10");
 
   // Array with self references.
   array->Set(index, release_array.get());
-  result = ScopedPPVar(ScopedPPVar::PassRef(),
-                       WriteAndRead(release_array.get()));
-  EXPECT_TRUE(TestEqual(release_array.get(), result.get()));
+  ASSERT_FALSE(WriteAndRead(release_array.get(), &result));
   // Break the self reference.
   array->Set(index, PP_MakeUndefined());
-  ArrayVar* result_array = ArrayVar::FromPPVar(result.get());
-  result_array->Set(index, PP_MakeUndefined());
 }
 
 }  // namespace proxy
diff --git a/ppapi/proxy/serialized_var.cc b/ppapi/proxy/serialized_var.cc
index 1217a83..72e7cc8 100644
--- a/ppapi/proxy/serialized_var.cc
+++ b/ppapi/proxy/serialized_var.cc
@@ -18,13 +18,20 @@
 namespace ppapi {
 namespace proxy {
 
+namespace {
+void DefaultHandleWriter(IPC::Message* m, const SerializedHandle& handle) {
+  IPC::ParamTraits<SerializedHandle>::Write(m, handle);
+}
+}  // namespace
+
 // SerializedVar::Inner --------------------------------------------------------
 
 SerializedVar::Inner::Inner()
     : serialization_rules_(NULL),
       var_(PP_MakeUndefined()),
       instance_(0),
-      cleanup_mode_(CLEANUP_NONE) {
+      cleanup_mode_(CLEANUP_NONE),
+      is_valid_var_(true) {
 #ifndef NDEBUG
   has_been_serialized_ = false;
   has_been_deserialized_ = false;
@@ -107,7 +114,24 @@ void SerializedVar::Inner::WriteToMessage(IPC::Message* m) const {
   DCHECK(!has_been_serialized_);
   has_been_serialized_ = true;
 #endif
-  RawVarDataGraph::Create(var_, instance_)->Write(m);
+  scoped_ptr<RawVarDataGraph> data = RawVarDataGraph::Create(var_, instance_);
+  if (data) {
+    m->WriteBool(true);  // Success.
+    data->Write(m, base::Bind(&DefaultHandleWriter));
+  } else {
+    m->WriteBool(false);  // Failure.
+  }
+}
+
+void SerializedVar::Inner::WriteDataToMessage(
+    IPC::Message* m,
+    const HandleWriter& handle_writer) const {
+  if (raw_var_data_) {
+    m->WriteBool(true);  // Success.
+    raw_var_data_->Write(m, handle_writer);
+  } else {
+    m->WriteBool(false);  // Failure.
+  }
 }
 
 bool SerializedVar::Inner::ReadFromMessage(const IPC::Message* m,
@@ -125,8 +149,15 @@ bool SerializedVar::Inner::ReadFromMessage(const IPC::Message* m,
 #endif
   // When reading, the dispatcher should be set when we get a Deserialize
   // call (which will supply a dispatcher).
-  raw_var_data_ = RawVarDataGraph::Read(m, iter);
-  return raw_var_data_.get() != NULL;
+  if (!m->ReadBool(iter, &is_valid_var_))
+      return false;
+  if (is_valid_var_) {
+    raw_var_data_ = RawVarDataGraph::Read(m, iter);
+    if (!raw_var_data_)
+      return false;
+  }
+
+  return true;
 }
 
 void SerializedVar::Inner::SetCleanupModeToEndSendPassRef() {
diff --git a/ppapi/proxy/serialized_var.h b/ppapi/proxy/serialized_var.h
index 98cafa0..0b8e796 100644
--- a/ppapi/proxy/serialized_var.h
+++ b/ppapi/proxy/serialized_var.h
@@ -80,12 +80,24 @@ class PPAPI_PROXY_EXPORT SerializedVar {
   void WriteToMessage(IPC::Message* m) const {
     inner_->WriteToMessage(m);
   }
+  // If ReadFromMessage has been called, WriteDataToMessage will write the var
+  // that has been read from ReadFromMessage back to a message. This is used
+  // when converting handles for use in NaCl.
+  void WriteDataToMessage(IPC::Message* m,
+                          const HandleWriter& handle_writer) const {
+    inner_->WriteDataToMessage(m, handle_writer);
+  }
   bool ReadFromMessage(const IPC::Message* m, PickleIterator* iter) {
     return inner_->ReadFromMessage(m, iter);
   }
 
-  RawVarDataGraph* raw_var_data() const {
-    return inner_->raw_var_data();
+  bool is_valid_var() const {
+    return inner_->is_valid_var();
+  }
+
+  // Returns the shared memory handles associated with this SerializedVar.
+  std::vector<SerializedHandle*> GetHandles() const {
+    return inner_->GetHandles();
   }
 
  protected:
@@ -110,8 +122,13 @@ class PPAPI_PROXY_EXPORT SerializedVar {
       serialization_rules_ = serialization_rules;
     }
 
-    RawVarDataGraph* raw_var_data() {
-      return raw_var_data_.get();
+    bool is_valid_var() const {
+      return is_valid_var_;
+    }
+
+    std::vector<SerializedHandle*> GetHandles() {
+      return (raw_var_data_ ? raw_var_data_->GetHandles() :
+          std::vector<SerializedHandle*>());
     }
 
     // See outer class's declarations above.
@@ -124,6 +141,8 @@ class PPAPI_PROXY_EXPORT SerializedVar {
     void ForceSetVarValueForTest(PP_Var value);
 
     void WriteToMessage(IPC::Message* m) const;
+    void WriteDataToMessage(IPC::Message* m,
+                            const HandleWriter& handle_writer) const;
     bool ReadFromMessage(const IPC::Message* m, PickleIterator* iter);
 
     // Sets the cleanup mode. See the CleanupMode enum below.
@@ -162,6 +181,9 @@ class PPAPI_PROXY_EXPORT SerializedVar {
 
     CleanupMode cleanup_mode_;
 
+    // If the var is not properly serialized, this will be false.
+    bool is_valid_var_;
+
 #ifndef NDEBUG
     // When being sent or received over IPC, we should only be serialized or
     // deserialized once. These flags help us assert this is true.
@@ -342,6 +364,7 @@ class PPAPI_PROXY_EXPORT SerializedVarReceiveInput {
 
   PP_Var Get(Dispatcher* dispatcher);
   PP_Var GetForInstance(Dispatcher* dispatcher, PP_Instance instance);
+  bool is_valid_var() { return serialized_.is_valid_var(); }
 
  private:
   const SerializedVar& serialized_;
diff --git a/ppapi/tests/test_post_message.cc b/ppapi/tests/test_post_message.cc
index 8c67ee5..944b5614 100644
--- a/ppapi/tests/test_post_message.cc
+++ b/ppapi/tests/test_post_message.cc
@@ -238,6 +238,16 @@ bool TestPostMessage::AddEchoingListener(const std::string& expression) {
   return true;
 }
 
+bool TestPostMessage::PostMessageFromJavaScript(const std::string& func) {
+  std::string js_code;
+  js_code += "var plugin = document.getElementById('plugin');"
+             "plugin.postMessage(";
+  js_code += func + "()";
+  js_code += "                      );";
+  instance_->EvalScript(js_code);
+  return true;
+}
+
 bool TestPostMessage::ClearListeners() {
   std::string js_code;
   js_code += "var plugin = document.getElementById('plugin');"
@@ -523,6 +533,7 @@ std::string TestPostMessage::TestSendingComplexVar() {
   // if we didn't run the 'SendInInit' test. All tests other than 'SendInInit'
   // should start with these.
   WaitForMessages();
+  message_data_.clear();
   ASSERT_TRUE(ClearListeners());
 
   pp::Var string(kTestString);
@@ -531,7 +542,6 @@ std::string TestPostMessage::TestSendingComplexVar() {
   dictionary.Set(pp::Var("bar"), string);
   dictionary.Set(pp::Var("abc"), pp::Var(kTestInt));
   dictionary.Set(pp::Var("def"), pp::Var());
-  dictionary.Set(pp::Var("dictionary"), dictionary);  // Self-reference.
 
   // Reference to array.
   pp::VarArray_Dev array;
@@ -544,15 +554,9 @@ std::string TestPostMessage::TestSendingComplexVar() {
   dictionary.Set(pp::Var("array-ref1"), array);
   dictionary.Set(pp::Var("array-ref2"), array);
 
-  // Set up a (dictionary -> array -> dictionary) cycle.
-  pp::VarArray_Dev array2;
-  array2.Set(0, dictionary);
-  dictionary.Set(pp::Var("array2"), array2);
-
   // Set up the JavaScript message event listener to echo the data part of the
   // message event back to us.
   ASSERT_TRUE(AddEchoingListener("message_event.data"));
-  message_data_.clear();
   instance_->PostMessage(dictionary);
   // PostMessage is asynchronous, so we should not receive a response yet.
   ASSERT_EQ(message_data_.size(), 0);
@@ -561,12 +565,36 @@ std::string TestPostMessage::TestSendingComplexVar() {
   pp::VarDictionary_Dev result(message_data_.back());
   ASSERT_TRUE(VarsEqual(dictionary, message_data_.back()));
 
+  WaitForMessages();
+  message_data_.clear();
+  ASSERT_TRUE(ClearListeners());
+
+  // Set up a (dictionary -> array -> dictionary) cycle. Cycles shouldn't be
+  // transmitted.
+  pp::VarArray_Dev array2;
+  array2.Set(0, dictionary);
+  dictionary.Set(pp::Var("array2"), array2);
+
+  ASSERT_TRUE(AddEchoingListener("message_event.data"));
+  instance_->PostMessage(dictionary);
+  // PostMessage is asynchronous, so we should not receive a response yet.
+  ASSERT_EQ(message_data_.size(), 0);
+  ASSERT_EQ(WaitForMessages(), 0);
+
   // Break the cycles.
-  dictionary.Delete(pp::Var("dictionary"));
   dictionary.Delete(pp::Var("array2"));
-  result.Delete(pp::Var("dictionary"));
-  result.Delete(pp::Var("array2"));
 
+  WaitForMessages();
+  message_data_.clear();
+  ASSERT_TRUE(ClearListeners());
+
+  // Test sending a cycle from JavaScript to the plugin.
+  ASSERT_TRUE(AddEchoingListener("message_event.data"));
+  PostMessageFromJavaScript("function() { var x = []; x[0] = x; return x; }");
+  ASSERT_EQ(message_data_.size(), 0);
+  ASSERT_EQ(WaitForMessages(), 0);
+
+  WaitForMessages();
   message_data_.clear();
   ASSERT_TRUE(ClearListeners());
 
diff --git a/ppapi/tests/test_post_message.h b/ppapi/tests/test_post_message.h
index 3ea69e8..2f2f628 100644
--- a/ppapi/tests/test_post_message.h
+++ b/ppapi/tests/test_post_message.h
@@ -34,6 +34,10 @@ class TestPostMessage : public TestCase {
   // Returns true on success, false on failure.
   bool AddEchoingListener(const std::string& expression);
 
+  // Posts a message from JavaScript to the plugin. |func| should be a
+  // JavaScript function which returns the variable to post.
+  bool PostMessageFromJavaScript(const std::string& func);
+
   // Clear any listeners that have been added using AddEchoingListener by
   // calling removeEventListener for each.
   // Returns true on success, false on failure.
diff --git a/webkit/plugins/ppapi/message_channel.cc b/webkit/plugins/ppapi/message_channel.cc
index 2984a92..76e0f3e 100644
--- a/webkit/plugins/ppapi/message_channel.cc
+++ b/webkit/plugins/ppapi/message_channel.cc
@@ -45,6 +45,12 @@ namespace ppapi {
 namespace {
 
 const char kPostMessage[] = "postMessage";
+const char kV8ToVarConversionError[] = "Failed to convert a PostMessage "
+    "argument from a JavaScript value to a PP_Var. It may have cycles or be of "
+    "an unsupported type.";
+const char kVarToV8ConversionError[] = "Failed to convert a PostMessage "
+    "argument from a PP_Var to a Javascript value. It may have cycles or be of "
+    "an unsupported type.";
 
 // Helper function to get the MessageChannel that is associated with an
 // NPObject*.
@@ -85,12 +91,16 @@ bool NPVariantToPPVar(const NPVariant* variant, PP_Var* result) {
           NPVARIANT_TO_STRING(*variant).UTF8Characters,
           NPVARIANT_TO_STRING(*variant).UTF8Length);
       return true;
-    case NPVariantType_Object:
-      V8VarConverter converter;
+    case NPVariantType_Object: {
       // Calling WebBindings::toV8Value creates a wrapper around NPVariant so it
       // shouldn't result in a deep copy.
-      return converter.FromV8Value(WebBindings::toV8Value(variant),
-                                   v8::Context::GetCurrent(), result);
+      v8::Handle<v8::Value> v8_value = WebBindings::toV8Value(variant);
+      if (!V8VarConverter::FromV8Value(v8_value, v8::Context::GetCurrent(),
+                                       result)) {
+        return false;
+      }
+      return true;
+    }
   }
   return false;
 }
@@ -182,7 +192,9 @@ bool MessageChannelInvoke(NPObject* np_obj, NPIdentifier name,
     if (message_channel) {
       PP_Var argument = PP_MakeUndefined();
       if (!NPVariantToPPVar(&args[0], &argument)) {
-        NOTREACHED();
+        PpapiGlobals::Get()->LogWithSource(
+            message_channel->instance()->pp_instance(),
+            PP_LOGLEVEL_ERROR, std::string(), kV8ToVarConversionError);
         return false;
       }
       message_channel->PostMessageToNative(argument);
@@ -346,10 +358,10 @@ void MessageChannel::PostMessageToJavaScript(PP_Var message_data) {
       container->element().document().frame()->mainWorldScriptContext();
   v8::Context::Scope context_scope(context);
 
-  v8::Local<v8::Value> v8_val;
-  V8VarConverter converter;
-  if (!converter.ToV8Value(message_data, context, &v8_val)) {
-    NOTREACHED();
+  v8::Handle<v8::Value> v8_val;
+  if (!V8VarConverter::ToV8Value(message_data, context, &v8_val)) {
+    PpapiGlobals::Get()->LogWithSource(instance_->pp_instance(),
+        PP_LOGLEVEL_ERROR, std::string(), kVarToV8ConversionError);
     return;
   }
 
diff --git a/webkit/plugins/ppapi/v8_var_converter.cc b/webkit/plugins/ppapi/v8_var_converter.cc
index 95b0e04..6f6baf7 100644
--- a/webkit/plugins/ppapi/v8_var_converter.cc
+++ b/webkit/plugins/ppapi/v8_var_converter.cc
@@ -27,6 +27,14 @@ using std::make_pair;
 
 namespace {
 
+template <class T>
+struct StackEntry {
+  StackEntry(T v) : val(v), sentinel(false) {}
+  T val;
+  // Used to track parent nodes on the stack while traversing the graph.
+  bool sentinel;
+};
+
 struct HashedHandle {
   HashedHandle(v8::Handle<v8::Object> h) : handle(h) {}
   size_t hash() const { return handle->GetIdentityHash(); }
@@ -54,14 +62,17 @@ inline size_t hash_value(const HashedHandle& handle) {
 
 namespace webkit {
 namespace ppapi {
+namespace V8VarConverter {
 
 namespace {
 
 // Maps PP_Var IDs to the V8 value handle they correspond to.
 typedef base::hash_map<int64_t, v8::Handle<v8::Value> > VarHandleMap;
+typedef base::hash_set<int64_t> ParentVarSet;
 
 // Maps V8 value handles to the PP_Var they correspond to.
 typedef base::hash_map<HashedHandle, ScopedPPVar> HandleVarMap;
+typedef base::hash_set<HashedHandle> ParentHandleSet;
 
 // Returns a V8 value which corresponds to a given PP_Var. If |var| is a
 // reference counted PP_Var type, and it exists in |visited_ids|, the V8 value
@@ -71,10 +82,13 @@ typedef base::hash_map<HashedHandle, ScopedPPVar> HandleVarMap;
 bool GetOrCreateV8Value(const PP_Var& var,
                         v8::Handle<v8::Value>* result,
                         bool* did_create,
-                        VarHandleMap* visited_ids) {
+                        VarHandleMap* visited_ids,
+                        ParentVarSet* parent_ids) {
   *did_create = false;
 
   if (::ppapi::VarTracker::IsVarTypeRefcounted(var.type)) {
+    if (parent_ids->count(var.value.as_id) != 0)
+      return false;
     VarHandleMap::iterator it = visited_ids->find(var.value.as_id);
     if (it != visited_ids->end()) {
       *result = it->second;
@@ -151,7 +165,8 @@ bool GetOrCreateV8Value(const PP_Var& var,
 bool GetOrCreateVar(v8::Handle<v8::Value> val,
                     PP_Var* result,
                     bool* did_create,
-                    HandleVarMap* visited_handles) {
+                    HandleVarMap* visited_handles,
+                    ParentHandleSet* parent_handles) {
   CHECK(!val.IsEmpty());
   *did_create = false;
 
@@ -159,6 +174,9 @@ bool GetOrCreateVar(v8::Handle<v8::Value> val,
   // we still add them to |visited_handles| so that the corresponding string
   // PP_Var created will be properly refcounted.
   if (val->IsObject() || val->IsString()) {
+    if (parent_handles->count(HashedHandle(val->ToObject())) != 0)
+      return false;
+
     HandleVarMap::const_iterator it = visited_handles->find(
         HashedHandle(val->ToObject()));
     if (it != visited_handles->end()) {
@@ -193,8 +211,10 @@ bool GetOrCreateVar(v8::Handle<v8::Value> val,
       *result = (new DictionaryVar())->GetPPVar();
     }
   } else {
-    NOTREACHED();
-    return false;
+    // Silently ignore the case where we can't convert to a Var as we may
+    // be trying to convert a type that doesn't have a corresponding
+    // PP_Var type.
+    return true;
   }
 
   *did_create = true;
@@ -206,31 +226,52 @@ bool GetOrCreateVar(v8::Handle<v8::Value> val,
   return true;
 }
 
-}  // namespace
-
-V8VarConverter::V8VarConverter() {
+bool CanHaveChildren(PP_Var var) {
+  return var.type == PP_VARTYPE_ARRAY || var.type == PP_VARTYPE_DICTIONARY;
 }
 
-bool V8VarConverter::ToV8Value(const PP_Var& var,
-                               v8::Handle<v8::Context> context,
-                               v8::Handle<v8::Value>* result) const {
+}  // namespace
+
+// To/FromV8Value use a stack-based DFS search to traverse V8/Var graph. Each
+// iteration, the top node on the stack examined. If the node has not been
+// visited yet (i.e. sentinel == false) then it is added to the list of parents
+// which contains all of the nodes on the path from the start node to the
+// current node. Each of the current nodes children are examined. If they appear
+// in the list of parents it means we have a cycle and we return NULL.
+// Otherwise, if they can have children, we add them to the stack. If the
+// node at the top of the stack has already been visited, then we pop it off the
+// stack and erase it from the list of parents.
+// static
+bool ToV8Value(const PP_Var& var,
+               v8::Handle<v8::Context> context,
+               v8::Handle<v8::Value>* result) {
   v8::Context::Scope context_scope(context);
   v8::HandleScope handle_scope;
 
   VarHandleMap visited_ids;
+  ParentVarSet parent_ids;
 
-  std::stack<PP_Var> stack;
-  stack.push(var);
+  std::stack<StackEntry<PP_Var> > stack;
+  stack.push(StackEntry<PP_Var>(var));
   v8::Handle<v8::Value> root;
   bool is_root = true;
 
   while (!stack.empty()) {
-    const PP_Var& current_var = stack.top();
+    const PP_Var& current_var = stack.top().val;
     v8::Handle<v8::Value> current_v8;
-    stack.pop();
+
+    if (stack.top().sentinel) {
+      stack.pop();
+      if (CanHaveChildren(current_var))
+        parent_ids.erase(current_var.value.as_id);
+      continue;
+    } else {
+      stack.top().sentinel = true;
+    }
+
     bool did_create = false;
     if (!GetOrCreateV8Value(current_var, &current_v8, &did_create,
-                            &visited_ids)) {
+                            &visited_ids, &parent_ids)) {
       return false;
     }
 
@@ -241,6 +282,7 @@ bool V8VarConverter::ToV8Value(const PP_Var& var,
 
     // Add child nodes to the stack.
     if (current_var.type == PP_VARTYPE_ARRAY) {
+      parent_ids.insert(current_var.value.as_id);
       ArrayVar* array_var = ArrayVar::FromPPVar(current_var);
       if (!array_var) {
         NOTREACHED();
@@ -253,14 +295,11 @@ bool V8VarConverter::ToV8Value(const PP_Var& var,
         const PP_Var& child_var = array_var->elements()[i].get();
         v8::Handle<v8::Value> child_v8;
         if (!GetOrCreateV8Value(child_var, &child_v8, &did_create,
-                                &visited_ids)) {
+                                &visited_ids, &parent_ids)) {
           return false;
         }
-        if (did_create &&
-            (child_var.type == PP_VARTYPE_DICTIONARY ||
-             child_var.type == PP_VARTYPE_ARRAY)) {
+        if (did_create && CanHaveChildren(child_var))
           stack.push(child_var);
-        }
         v8::TryCatch try_catch;
         v8_array->Set(static_cast<uint32>(i), child_v8);
         if (try_catch.HasCaught()) {
@@ -269,6 +308,7 @@ bool V8VarConverter::ToV8Value(const PP_Var& var,
         }
       }
     } else if (current_var.type == PP_VARTYPE_DICTIONARY) {
+      parent_ids.insert(current_var.value.as_id);
       DictionaryVar* dict_var = DictionaryVar::FromPPVar(current_var);
       if (!dict_var) {
         NOTREACHED();
@@ -285,14 +325,11 @@ bool V8VarConverter::ToV8Value(const PP_Var& var,
         const PP_Var& child_var = iter->second.get();
         v8::Handle<v8::Value> child_v8;
         if (!GetOrCreateV8Value(child_var, &child_v8, &did_create,
-                                &visited_ids)) {
+                                &visited_ids, &parent_ids)) {
           return false;
         }
-        if (did_create &&
-            (child_var.type == PP_VARTYPE_DICTIONARY ||
-             child_var.type == PP_VARTYPE_ARRAY)) {
+        if (did_create && CanHaveChildren(child_var))
           stack.push(child_var);
-        }
         v8::TryCatch try_catch;
         v8_object->Set(v8::String::New(key.c_str(), key.length()), child_v8);
         if (try_catch.HasCaught()) {
@@ -308,26 +345,36 @@ bool V8VarConverter::ToV8Value(const PP_Var& var,
   return true;
 }
 
-bool V8VarConverter::FromV8Value(v8::Handle<v8::Value> val,
-                                 v8::Handle<v8::Context> context,
-                                 PP_Var* result) const {
+bool FromV8Value(v8::Handle<v8::Value> val,
+                 v8::Handle<v8::Context> context,
+                 PP_Var* result) {
   v8::Context::Scope context_scope(context);
   v8::HandleScope handle_scope;
 
   HandleVarMap visited_handles;
+  ParentHandleSet parent_handles;
 
-  std::stack<v8::Handle<v8::Value> > stack;
-  stack.push(val);
+  std::stack<StackEntry<v8::Handle<v8::Value> > > stack;
+  stack.push(StackEntry<v8::Handle<v8::Value> >(val));
   ScopedPPVar root;
   bool is_root = true;
 
   while (!stack.empty()) {
-    v8::Handle<v8::Value> current_v8 = stack.top();
+    v8::Handle<v8::Value> current_v8 = stack.top().val;
     PP_Var current_var;
-    stack.pop();
+
+    if (stack.top().sentinel) {
+      stack.pop();
+      if (current_v8->IsObject())
+        parent_handles.erase(HashedHandle(current_v8->ToObject()));
+      continue;
+    } else {
+      stack.top().sentinel = true;
+    }
+
     bool did_create = false;
     if (!GetOrCreateVar(current_v8, &current_var, &did_create,
-                        &visited_handles)) {
+                        &visited_handles, &parent_handles)) {
       return false;
     }
 
@@ -340,6 +387,7 @@ bool V8VarConverter::FromV8Value(v8::Handle<v8::Value> val,
     if (current_var.type == PP_VARTYPE_ARRAY) {
       DCHECK(current_v8->IsArray());
       v8::Handle<v8::Array> v8_array = current_v8.As<v8::Array>();
+      parent_handles.insert(HashedHandle(v8_array));
 
       ArrayVar* array_var = ArrayVar::FromPPVar(current_var);
       if (!array_var) {
@@ -358,11 +406,8 @@ bool V8VarConverter::FromV8Value(v8::Handle<v8::Value> val,
 
         PP_Var child_var;
         if (!GetOrCreateVar(child_v8, &child_var, &did_create,
-                            &visited_handles)) {
-          // Silently ignore the case where we can't convert to a Var as we may
-          // be trying to convert a type that doesn't have a corresponding
-          // PP_Var type.
-          continue;
+                            &visited_handles, &parent_handles)) {
+          return false;
         }
         if (did_create && child_v8->IsObject())
           stack.push(child_v8);
@@ -372,6 +417,7 @@ bool V8VarConverter::FromV8Value(v8::Handle<v8::Value> val,
     } else if (current_var.type == PP_VARTYPE_DICTIONARY) {
       DCHECK(current_v8->IsObject());
       v8::Handle<v8::Object> v8_object = current_v8->ToObject();
+      parent_handles.insert(HashedHandle(v8_object));
 
       DictionaryVar* dict_var = DictionaryVar::FromPPVar(current_var);
       if (!dict_var) {
@@ -403,8 +449,8 @@ bool V8VarConverter::FromV8Value(v8::Handle<v8::Value> val,
 
         PP_Var child_var;
         if (!GetOrCreateVar(child_v8, &child_var, &did_create,
-                            &visited_handles)) {
-          continue;
+                            &visited_handles, &parent_handles)) {
+          return false;
         }
         if (did_create && child_v8->IsObject())
           stack.push(child_v8);
@@ -419,5 +465,6 @@ bool V8VarConverter::FromV8Value(v8::Handle<v8::Value> val,
   return true;
 }
 
+}  // namespace V8VarConverter
 }  // namespace ppapi
 }  // namespace webkit
diff --git a/webkit/plugins/ppapi/v8_var_converter.h b/webkit/plugins/ppapi/v8_var_converter.h
index c19644c..9dcae4f8 100644
--- a/webkit/plugins/ppapi/v8_var_converter.h
+++ b/webkit/plugins/ppapi/v8_var_converter.h
@@ -14,27 +14,21 @@
 
 namespace webkit {
 namespace ppapi {
-
-// Class to convert between PP_Vars and V8 values.
-class WEBKIT_PLUGINS_EXPORT V8VarConverter {
- public:
-  V8VarConverter();
-
-  // Converts the given PP_Var to a v8::Value. True is returned upon success.
-  bool ToV8Value(const PP_Var& var,
-                 v8::Handle<v8::Context> context,
-                 v8::Handle<v8::Value>* result) const;
-  // Converts the given v8::Value to a PP_Var. True is returned upon success.
-  // Every PP_Var in the reference graph of which |result| is apart will have
-  // a refcount equal to the number of references to it in the graph. |result|
-  // will have one additional reference.
-  bool FromV8Value(v8::Handle<v8::Value> val,
-                   v8::Handle<v8::Context> context,
-                   PP_Var* result) const;
-
-  DISALLOW_COPY_AND_ASSIGN(V8VarConverter);
-};
-
+namespace V8VarConverter {
+
+// Converts the given PP_Var to a v8::Value. True is returned upon success.
+bool WEBKIT_PLUGINS_EXPORT ToV8Value(const PP_Var& var,
+                                     v8::Handle<v8::Context> context,
+                                     v8::Handle<v8::Value>* result);
+// Converts the given v8::Value to a PP_Var. True is returned upon success.
+// Every PP_Var in the reference graph of which |result| is apart will have
+// a refcount equal to the number of references to it in the graph. |result|
+// will have one additional reference.
+bool WEBKIT_PLUGINS_EXPORT FromV8Value(v8::Handle<v8::Value> val,
+                                       v8::Handle<v8::Context> context,
+                                       PP_Var* result);
+
+}  // namespace V8VarConverter
 }  // namespace ppapi
 }  // namespace webkit
 
diff --git a/webkit/plugins/ppapi/v8_var_converter_unittest.cc b/webkit/plugins/ppapi/v8_var_converter_unittest.cc
index 2198cf8..fc4c841 100644
--- a/webkit/plugins/ppapi/v8_var_converter_unittest.cc
+++ b/webkit/plugins/ppapi/v8_var_converter_unittest.cc
@@ -152,17 +152,16 @@ class V8VarConverterTest : public testing::Test {
 
  protected:
   bool RoundTrip(const PP_Var& var, PP_Var* result) {
-    V8VarConverter converter;
     v8::HandleScope handle_scope(isolate_);
     v8::Context::Scope context_scope(isolate_, context_);
     v8::Local<v8::Context> context =
         v8::Local<v8::Context>::New(isolate_, context_);
     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;
   }
@@ -271,29 +270,61 @@ TEST_F(V8VarConverterTest, DictionaryArrayRoundTripTest) {
   array2->Set(0, PP_MakeInt32(100));
   dictionary->SetWithStringKey("9", release_array2.get());
   EXPECT_TRUE(RoundTripAndCompare(array->GetPPVar()));
+}
+
+TEST_F(V8VarConverterTest, Cycles) {
+  // Check that cycles aren't converted.
+  v8::Context::Scope context_scope(context_);
+  v8::HandleScope handle_scope;
+
+  // 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("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");
-
-  // 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());
+    // 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) {
@@ -325,10 +356,9 @@ TEST_F(V8VarConverterTest, StrangeDictionaryKeyTest) {
     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);