Update client side navigator.connect API to use ServicePortCollection [2/3]

content/browser/navigator_connect/service_port_service_impl.cc

Index: content/browser/navigator_connect/service_port_service_impl.cc
diff --git a/content/browser/navigator_connect/service_port_service_impl.cc b/content/browser/navigator_connect/service_port_service_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0d9c70cc5e7189597c11231fe799032db2c2e348
--- /dev/null
+++ b/content/browser/navigator_connect/service_port_service_impl.cc
@@ -0,0 +1,128 @@
+// Copyright 2015 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/browser/navigator_connect/service_port_service_impl.h"
+
+#include "content/browser/message_port_message_filter.h"
+#include "content/browser/message_port_service.h"
+#include "content/browser/navigator_connect/navigator_connect_context_impl.h"
+#include "content/common/service_port_type_converters.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/common/message_port_types.h"
+#include "mojo/common/common_type_converters.h"
+#include "url/gurl.h"
+
+namespace content {
+
+// static
+void ServicePortServiceImpl::Create(
+    const scoped_refptr<NavigatorConnectContextImpl>& navigator_connect_context,
+    const scoped_refptr<MessagePortMessageFilter>& message_port_message_filter,
+    mojo::InterfaceRequest<ServicePortService> request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&ServicePortServiceImpl::CreateOnIOThread,
+                 navigator_connect_context, message_port_message_filter,
+                 base::Passed(&request)));
+}
+
+ServicePortServiceImpl::~ServicePortServiceImpl() {
+  // Should always be destroyed on the IO thread, but can't check that with
+  // DCHECK_CURRENTLY_ON because this class could be destroyed during thread
+  // shutdown, at which point that check doesn't work.
+  navigator_connect_context_->ServicePortServiceDestroyed(this);
+}
+
+void ServicePortServiceImpl::PostMessageToClient(
+    int port_id,
+    const MessagePortMessage& message,
+    const std::vector<TransferredMessagePort>& sent_message_ports) {
+  DCHECK(client_.get());
+  // Hold messages on transferred message ports. Normally this wouldn't be
+  // needed, but since MessagePort uses regular IPC while this class uses mojo,
+  // without holding messages mojo IPC might overtake regular IPC resulting in a
+  // non-functional port. When  WebMessagePortChannelImpl instances are
+  // constructed in the renderer, they will send
+  // MessagePortHostMsg_ReleaseMessages to release messages.
+  for (const auto& port : sent_message_ports)
+    MessagePortService::GetInstance()->HoldMessages(port.id);
+
+  std::vector<int> new_routing_ids;
+  message_port_message_filter_->UpdateMessagePortsWithNewRoutes(
+      sent_message_ports, &new_routing_ids);
+  client_->PostMessage(
+      port_id, mojo::String::From(message.message_as_string),
+      mojo::Array<MojoTransferredMessagePortPtr>::From(sent_message_ports),
+      mojo::Array<int32_t>::From(new_routing_ids));
+}
+
+// static
+void ServicePortServiceImpl::CreateOnIOThread(
+    const scoped_refptr<NavigatorConnectContextImpl>& navigator_connect_context,
+    const scoped_refptr<MessagePortMessageFilter>& message_port_message_filter,
+    mojo::InterfaceRequest<ServicePortService> request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  new ServicePortServiceImpl(navigator_connect_context,
+                             message_port_message_filter, request.Pass());
+}
+
+ServicePortServiceImpl::ServicePortServiceImpl(
+    const scoped_refptr<NavigatorConnectContextImpl>& navigator_connect_context,
+    const scoped_refptr<MessagePortMessageFilter>& message_port_message_filter,
+    mojo::InterfaceRequest<ServicePortService> request)
+    : binding_(this, request.Pass()),
+      navigator_connect_context_(navigator_connect_context),
+      message_port_message_filter_(message_port_message_filter),
+      weak_ptr_factory_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+}
+
+void ServicePortServiceImpl::SetClient(ServicePortServiceClientPtr client) {
+  DCHECK(!client_.get());
+  // TODO(mek): Set ErrorHandler to listen for errors.
+  client_ = client.Pass();
+}
+
+void ServicePortServiceImpl::Connect(const mojo::String& target_url,
+                                     const mojo::String& origin,
+                                     const ConnectCallback& callback) {
+  navigator_connect_context_->Connect(
+      GURL(target_url), GURL(origin), this,
+      base::Bind(&ServicePortServiceImpl::OnConnectResult,
+                 weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void ServicePortServiceImpl::PostMessage(
+    int32_t port_id,

[scheib, 2015/06/24 05:20:06]

port_id should somewhere be verified or by implementation details be limited to
only work for ports that the renderer has a relationship with. At this point in
review I'm not clear on where that is. Probably add a comment in this method
explaining how security works for limiting access to ports. 

[Marijn Kruisselbrink, 2015/06/24 17:33:53]

This is kind of similar to the existing problems with message ports
(http://crbug.com/490222) where there currently are no checks for any of this,
and it's trivially easy for any renderer to transfer any message port to itself
to claim full ownership of it. It's a bit less severe here since this code isn't
going to support transferring of ports, which also should make it easier to add
these checks. It's just a bit tricky to do while ServicePort still is built on
top of MessagePort, but I've added a TODO to make sure this gets addressed.

+    const mojo::String& message,
+    mojo::Array<MojoTransferredMessagePortPtr> ports) {
+  std::vector<TransferredMessagePort> transferred_ports =
+      ports.To<std::vector<TransferredMessagePort>>();
+
+  MessagePortService* mps = MessagePortService::GetInstance();
+  // First call QueueMessages for all transferred ports, since the ports haven't

[scheib, 2015/06/24 05:20:06]

First, ...

[Marijn Kruisselbrink, 2015/06/24 17:33:53]

Done

+  // send their own IPC for that.

[scheib, 2015/06/24 05:20:06]

grammar fix, send->sent I think?

[Marijn Kruisselbrink, 2015/06/24 17:33:53]

Done

+  for (const TransferredMessagePort& port : transferred_ports) {
+    mps->QueueMessages(port.id);
+  }
+
+  // Second pass of the actual to MessagePortService now ServicePort instances

[scheib, 2015/06/24 05:20:06]

Second, pass ... 

[Marijn Kruisselbrink, 2015/06/24 17:33:53]

Done

+  // are still backed by MessagePort.
+  mps->PostMessage(port_id, MessagePortMessage(message.To<base::string16>()),
+                   transferred_ports);
+}
+
+void ServicePortServiceImpl::ClosePort(int32_t port_id) {
+  MessagePortService::GetInstance()->Destroy(port_id);
+}
+
+void ServicePortServiceImpl::OnConnectResult(const ConnectCallback& callback,
+                                             int message_port_id,
+                                             bool success) {
+  callback.Run(success ? SERVICE_PORT_ERROR_SUCCESS : SERVICE_PORT_ERROR_ABORT,
+               message_port_id);
+}
+
+}  // namespace content