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

webkit/plugins/ppapi/message_channel.cc

Index: webkit/plugins/ppapi/message_channel.cc
diff --git a/webkit/plugins/ppapi/message_channel.cc b/webkit/plugins/ppapi/message_channel.cc
index bbb3eb2274edcdda992eb1c92b98b8a77030a3c4..75c089a7e18cbe675a582c29121a9c06142d6173 100644
--- a/webkit/plugins/ppapi/message_channel.cc
+++ b/webkit/plugins/ppapi/message_channel.cc
@@ -45,6 +45,21 @@ 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: ";
+
+std::string MakeV8ConversionError(v8::Handle<v8::Value> v8_value) {
+  v8::String::Utf8Value utf8(v8_value->ToString());
+  return kV8ToVarConversionError + std::string(*utf8, utf8.length());

[dmichael (off chromium), 2013/06/04 16:59:10]

Are you actually converting the v8_value to a string? That can probably be
pretty big, can't it? I think I'd rather just not give that level of detail.
WDYT?

[raymes, 2013/06/04 19:36:06]

That's ok with me.

+}
+
+std::string MakePPVarConversionError(PP_Var var) {
+  return kVarToV8ConversionError + ::ppapi::Var::PPVarToLogString(var);
+}
 
 // Helper function to get the MessageChannel that is associated with an
 // NPObject*.
@@ -63,7 +78,9 @@ bool IdentifierIsPostMessage(NPIdentifier identifier) {
   return WebBindings::getStringIdentifier(kPostMessage) == identifier;
 }
 
-bool NPVariantToPPVar(const NPVariant* variant, PP_Var* result) {
+bool NPVariantToPPVar(const NPVariant* variant,
+                      PP_Var* result,
+                      std::string* error) {
   switch (variant->type) {
     case NPVariantType_Void:
       *result = PP_MakeUndefined();
@@ -85,13 +102,19 @@ 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: {
+      V8VarConverter converter(false);
       // 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 (!converter.FromV8Value(v8_value, v8::Context::GetCurrent(), result)) {
+        *error = MakeV8ConversionError(v8_value);
+        return false;
+      }
+      return true;
+    }
   }
+  *error = MakeV8ConversionError(WebBindings::toV8Value(variant));
   return false;
 }
 
@@ -181,13 +204,11 @@ bool MessageChannelInvoke(NPObject* np_obj, NPIdentifier name,
     MessageChannel* message_channel = ToMessageChannel(np_obj);
     if (message_channel) {
       PP_Var argument = PP_MakeUndefined();
-      if (!NPVariantToPPVar(&args[0], &argument)) {
-        NOTREACHED();
-        return false;
-      }
-      message_channel->PostMessageToNative(argument);
+      std::string error;
+      bool success = NPVariantToPPVar(&args[0], &argument, &error);
+      message_channel->PostMessageToNative(argument, success, error);
       PpapiGlobals::Get()->GetVarTracker()->ReleaseVar(argument);
-      return true;
+      return success;
     } else {
       return false;
     }
@@ -346,11 +367,13 @@ 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;
+  v8::Handle<v8::Value> v8_val;
+  V8VarConverter converter(false);
   if (!converter.ToV8Value(message_data, context, &v8_val)) {
-    NOTREACHED();
-    return;
+    PpapiGlobals::Get()->LogWithSource(instance_->pp_instance(),
+        PP_LOGLEVEL_ERROR, std::string(),
+        MakePPVarConversionError(message_data));
+    v8_val = v8::Undefined();
   }
 
   // This is for backward compatibility. It usually makes sense for us to return
@@ -457,7 +480,14 @@ void MessageChannel::PostMessageToJavaScriptImpl(
   container->element().dispatchEvent(msg_event);
 }
 
-void MessageChannel::PostMessageToNative(PP_Var message_data) {
+void MessageChannel::PostMessageToNative(PP_Var message_data,
+                                         bool success,
+                                         const std::string& error_msg) {
+  if (!success) {
+    PpapiGlobals::Get()->LogWithSource(instance_->pp_instance(),
+        PP_LOGLEVEL_ERROR, std::string(), error_msg);
+    message_data = PP_MakeUndefined();

[dmichael (off chromium), 2013/06/04 16:59:10]

This feels kind of klugy to me. My first choice is to not send "Undefined" in
the case of error. My second choice, if you disagree with that, would be to do
the logging outside of this function, and pass this function an Undefined
instead of doing this. I think it's easier to understand if its only
responsibility is sending |message_data|.

[raymes, 2013/06/04 19:36:06]

This was kludgy. Hopefully my change seems nicer.

+  }
   if (instance_->module()->IsProxied()) {
     // In the proxied case, the copy will happen via serializiation, and the
     // message is asynchronous. Therefore there's no need to copy the Var, nor