Optionally have MessagePort pass data as base::Value, part 1.

content/child/webmessageportchannel_impl.cc

Index: content/child/webmessageportchannel_impl.cc
diff --git a/content/child/webmessageportchannel_impl.cc b/content/child/webmessageportchannel_impl.cc
index 725e5956288946d470953bbd991a0bc7bf03fbe1..2c38f3212556ddcf748fb70bc3bdba6856132ded 100644
--- a/content/child/webmessageportchannel_impl.cc
+++ b/content/child/webmessageportchannel_impl.cc
@@ -6,11 +6,15 @@
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/values.h"
 #include "content/child/child_process.h"
 #include "content/child/child_thread_impl.h"
 #include "content/common/message_port_messages.h"
+#include "content/public/child/v8_value_converter.h"
 #include "third_party/WebKit/public/platform/WebMessagePortChannelClient.h"
 #include "third_party/WebKit/public/platform/WebString.h"
+#include "third_party/WebKit/public/web/WebSerializedScriptValue.h"
+#include "v8/include/v8.h"
 
 using blink::WebMessagePortChannel;
 using blink::WebMessagePortChannelArray;
@@ -24,6 +28,7 @@ WebMessagePortChannelImpl::WebMessagePortChannelImpl(
     : client_(NULL),
       route_id_(MSG_ROUTING_NONE),
       message_port_id_(MSG_ROUTING_NONE),
+      send_messages_as_values_(false),
       main_thread_task_runner_(main_thread_task_runner) {
   AddRef();
   Init();
@@ -36,6 +41,7 @@ WebMessagePortChannelImpl::WebMessagePortChannelImpl(
     : client_(NULL),
       route_id_(route_id),
       message_port_id_(message_port_id),
+      send_messages_as_values_(false),
       main_thread_task_runner_(main_thread_task_runner) {
   AddRef();
   Init();
@@ -113,24 +119,32 @@ void WebMessagePortChannelImpl::destroy() {
 }
 
 void WebMessagePortChannelImpl::postMessage(
-    const WebString& message,
+    const WebString& message_as_string,
     WebMessagePortChannelArray* channels) {
+  MessagePortMessage message(message_as_string);
+  if (send_messages_as_values_) {
+    blink::WebSerializedScriptValue serialized_value =
+        blink::WebSerializedScriptValue::fromString(message_as_string);
+    v8::Handle<v8::Value> v8_value = serialized_value.deserialize();
+    scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
+    scoped_ptr<base::Value> message_as_value(converter->FromV8Value(
+        v8_value, v8::Isolate::GetCurrent()->GetCurrentContext()));
+    message = MessagePortMessage(message_as_value.get());
+  }
   if (!main_thread_task_runner_->BelongsToCurrentThread()) {
     main_thread_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(
-            &WebMessagePortChannelImpl::PostMessage, this,
-            static_cast<base::string16>(message), channels));
+        FROM_HERE, base::Bind(&WebMessagePortChannelImpl::PostMessage, this,
+                              message, channels));
   } else {
     PostMessage(message, channels);
   }
 }
 
 void WebMessagePortChannelImpl::PostMessage(
-    const base::string16& message,
+    const MessagePortMessage& message,
     WebMessagePortChannelArray* channels) {
   IPC::Message* msg = new MessagePortHostMsg_PostMessage(
-      message_port_id_, message, ExtractMessagePortIDs(channels));
+        message_port_id_, message, ExtractMessagePortIDs(channels));
   Send(msg);
 }
 
@@ -141,7 +155,20 @@ bool WebMessagePortChannelImpl::tryGetMessage(
   if (message_queue_.empty())
     return false;
 
-  *message = message_queue_.front().message;
+  const base::Value* message_as_value;
+  if (message_queue_.front().message.message_as_value.Get(0,

[scheib, 2015/02/18 20:58:55]

Seems a bit implicit, would read cleanly with something like
MessagePortMessage::StoresAValue().

[Marijn Kruisselbrink, 2015/02/26 23:12:54]

Done.

+                                                          &message_as_value)) {
+    v8::HandleScope handle_scope(client_->scriptIsolate());
+    v8::Context::Scope context_scope(client_->scriptContext());
+    scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
+    v8::Handle<v8::Value> v8_value =
+        converter->ToV8Value(message_as_value, client_->scriptContext());

[adamk, 2015/02/18 21:34:46]

This call can have side effects if there are setters on builtin prototypes (see
http://crbug.com/459731 for a related example). See 

https://code.google.com/p/chromium/codesearch#chromium/src/content/child/v8_v...

for how V8ValueConverter handles this. I'm worried that allowing author JS to
run at this time could have unforeseen consequences. Even if V8ValueConverter
were made safe, my reading of the SerializedScriptValue implementation in Blink
suggests that, for example, serializing an Array with holes can cause getters on
the prototype to be run:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

("Dense" is defined as "6 * propertyCount >= length", and the existing code
actually gets the property count wrong, from what I can tell).

In short, it looks to me like a variety of changes would need to be made in
V8ValueConverter and SerializedScriptValue before the conversions done here
could be done transparently, without the possibility of invoking author script.

So I'm inclined to push a bit harder on trying to come up with a more direct
SerializedScriptValue <-> base::Value translation that leaves out the JavaScript
middleman. You mentioned in the other patch that this would require significant
refactoring; do you have a sketch of that design, and how much work it might be?

+    blink::WebSerializedScriptValue serialized_value =
+        blink::WebSerializedScriptValue::serialize(v8_value);
+    *message = serialized_value.toString();
+  } else {
+    *message = message_queue_.front().message.message_as_string;
+  }
   const std::vector<WebMessagePortChannelImpl*>& channel_array =
       message_queue_.front().ports;
   WebMessagePortChannelArray result_ports(channel_array.size());
@@ -230,7 +257,7 @@ bool WebMessagePortChannelImpl::OnMessageReceived(const IPC::Message& message) {
 }
 
 void WebMessagePortChannelImpl::OnMessage(
-    const base::string16& message,
+    const MessagePortMessage& message,
     const std::vector<int>& sent_message_port_ids,
     const std::vector<int>& new_routing_ids) {
   base::AutoLock auto_lock(lock_);
@@ -240,8 +267,7 @@ void WebMessagePortChannelImpl::OnMessage(
     msg.ports.resize(sent_message_port_ids.size());
     for (size_t i = 0; i < sent_message_port_ids.size(); ++i) {
       msg.ports[i] = new WebMessagePortChannelImpl(
-          new_routing_ids[i],
-          sent_message_port_ids[i],
+          new_routing_ids[i], sent_message_port_ids[i],
           main_thread_task_runner_.get());
     }
   }
@@ -259,7 +285,7 @@ void WebMessagePortChannelImpl::OnMessagesQueued() {
     base::AutoLock auto_lock(lock_);
     queued_messages.reserve(message_queue_.size());
     while (!message_queue_.empty()) {
-      base::string16 message = message_queue_.front().message;
+      MessagePortMessage message = message_queue_.front().message;
       const std::vector<WebMessagePortChannelImpl*>& channel_array =
           message_queue_.front().ports;
       std::vector<int> port_ids(channel_array.size());