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

webkit/plugins/ppapi/message_channel.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/message_channel.h"
 
 #include <cstdlib>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 27 matching lines @@
 using WebKit::WebPluginContainer;
 using WebKit::WebSerializedScriptValue;
 
 namespace webkit {
 
 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*.
 MessageChannel* ToMessageChannel(NPObject* object) {
   return static_cast<MessageChannel::MessageChannelNPObject*>(object)->
       message_channel;
 }
 
 NPObject* ToPassThroughObject(NPObject* object) {
   MessageChannel* channel = ToMessageChannel(object);
   return channel ? channel->passthrough_object() : NULL;
 }
 
 // Helper function to determine if a given identifier is equal to kPostMessage.
 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();
       return true;
     case NPVariantType_Null:
       *result = PP_MakeNull();
       return true;
     case NPVariantType_Bool:
       *result = PP_MakeBool(PP_FromBool(NPVARIANT_TO_BOOLEAN(*variant)));
       return true;
     case NPVariantType_Int32:
       *result = PP_MakeInt32(NPVARIANT_TO_INT32(*variant));
       return true;
     case NPVariantType_Double:
       *result = PP_MakeDouble(NPVARIANT_TO_DOUBLE(*variant));
       return true;
     case NPVariantType_String:
       *result = StringVar::StringToPPVar(
           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;
 }
 
 // Copy a PP_Var in to a PP_Var that is appropriate for sending via postMessage.
 // This currently just copies the value.  For a string Var, the result is a
 // PP_Var with the a copy of |var|'s string contents and a reference count of 1.
 PP_Var CopyPPVar(const PP_Var& var) {
   switch (var.type) {
     case PP_VARTYPE_UNDEFINED:
     case PP_VARTYPE_NULL:
@@ skipping 69 matching lines @@
                           const NPVariant* args, uint32 arg_count,
                           NPVariant* result) {
   if (!np_obj)
     return false;
 
   // We only handle a function called postMessage.
   if (IdentifierIsPostMessage(name) && (arg_count == 1)) {
     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;
     }
   }
   // Other method calls we will pass to the passthrough object, if we have one.
   NPObject* passthrough = ToPassThroughObject(np_obj);
   if (passthrough) {
     return WebBindings::invoke(NULL, passthrough, name, args, arg_count,
                                result);
   }
@@ skipping 138 matching lines @@
   WebPluginContainer* container = instance_->container();
   // It's possible that container() is NULL if the plugin has been removed from
   // the DOM (but the PluginInstance is not destroyed yet).
   if (!container)
     return;
 
   v8::Local<v8::Context> context =
       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
   // a string object rather than a string primitive because it allows multiple
   // references to the same string (as with PP_Var strings). However, prior to
   // implementing dictionary and array, vars we would return a string primitive
   // here. Changing it to an object now will break existing code that uses
   // strict comparisons for strings returned from PostMessage. e.g. x === "123"
   // will no longer return true. So if the only value to return is a string
   // object, just return the string primitive.
@@ skipping 86 matching lines @@
   //     messages:
   //      http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html
   //     This currently behaves like Web Workers. On Firefox, Chrome, and Safari
   //     at least, postMessage on Workers does not provide the origin or source.
   //     TODO(dmichael):  Add origin if we change to a more iframe-like origin
   //                      policy (see crbug.com/81537)
 
   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
     // to PostTask.
     PostMessageToNativeImpl(message_data);
   } else {
     // Make a copy of the message data for the Task we will run.
     PP_Var var_copy(CopyPPVar(message_data));
 
     base::MessageLoop::current()->PostTask(
@@ skipping 25 matching lines @@
   // invokes:
   //   SetPassthroughObject(passthrough_object());
   if (passthrough_object_)
     WebBindings::releaseObject(passthrough_object_);
 
   passthrough_object_ = passthrough;
 }
 
 }  // namespace ppapi
 }  // namespace webkit