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

content/child/webmessageportchannel_impl.cc

 // Copyright (c) 2011 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 "content/child/webmessageportchannel_impl.h"
 
 #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;
 using blink::WebMessagePortChannelClient;
 using blink::WebString;
 
 namespace content {
 
 WebMessagePortChannelImpl::WebMessagePortChannelImpl(
     const scoped_refptr<base::SingleThreadTaskRunner>& main_thread_task_runner)
     : 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();
 }
 
 WebMessagePortChannelImpl::WebMessagePortChannelImpl(
     int route_id,
     int message_port_id,
     const scoped_refptr<base::SingleThreadTaskRunner>& main_thread_task_runner)
     : 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();
 }
 
 WebMessagePortChannelImpl::~WebMessagePortChannelImpl() {
   // If we have any queued messages with attached ports, manually destroy them.
   while (!message_queue_.empty()) {
     const std::vector<WebMessagePortChannelImpl*>& channel_array =
         message_queue_.front().ports;
@@ skipping 57 matching lines @@
 
 void WebMessagePortChannelImpl::destroy() {
   setClient(NULL);
 
   // Release the object on the main thread, since the destructor might want to
   // send an IPC, and that has to happen on the main thread.
   main_thread_task_runner_->ReleaseSoon(FROM_HERE, this);
 }
 
 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);
 }
 
 bool WebMessagePortChannelImpl::tryGetMessage(
     WebString* message,
     WebMessagePortChannelArray& channels) {
   base::AutoLock auto_lock(lock_);
   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());
   for (size_t i = 0; i < channel_array.size(); i++) {
     result_ports[i] = channel_array[i];
   }
 
   channels.swap(result_ports);
   message_queue_.pop();
   return true;
@@ skipping 68 matching lines @@
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(WebMessagePortChannelImpl, message)
     IPC_MESSAGE_HANDLER(MessagePortMsg_Message, OnMessage)
     IPC_MESSAGE_HANDLER(MessagePortMsg_MessagesQueued, OnMessagesQueued)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 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_);
   Message msg;
   msg.message = message;
   if (!sent_message_port_ids.empty()) {
     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());
     }
   }
 
   bool was_empty = message_queue_.empty();
   message_queue_.push(msg);
   if (client_ && was_empty)
     client_->messageAvailable();
 }
 
 void WebMessagePortChannelImpl::OnMessagesQueued() {
   std::vector<QueuedMessage> queued_messages;
 
   {
     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());
       for (size_t i = 0; i < channel_array.size(); ++i) {
         port_ids[i] = channel_array[i]->message_port_id();
         channel_array[i]->QueueMessages();
       }
       queued_messages.push_back(std::make_pair(message, port_ids));
       message_queue_.pop();
     }
   }
 
   Send(new MessagePortHostMsg_SendQueuedMessages(
       message_port_id_, queued_messages));
 
   message_port_id_ = MSG_ROUTING_NONE;
 
   Release();
   ChildProcess::current()->ReleaseProcess();
 }
 
 WebMessagePortChannelImpl::Message::Message() {}
 
 WebMessagePortChannelImpl::Message::~Message() {}
 
 }  // namespace content