[Extensions] Begin making Extension port initialization asynchronous

extensions/renderer/messaging_bindings.cc

Index: extensions/renderer/messaging_bindings.cc
diff --git a/extensions/renderer/messaging_bindings.cc b/extensions/renderer/messaging_bindings.cc
index db61703c19bd1a25280fd5dc10305ae0c216d946..80091505f6be28dbcd30a2567e299da22d61f476 100644
--- a/extensions/renderer/messaging_bindings.cc
+++ b/extensions/renderer/messaging_bindings.cc
@@ -12,6 +12,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/lazy_instance.h"
 #include "base/message_loop/message_loop.h"
 #include "base/values.h"
 #include "content/public/child/v8_value_converter.h"
@@ -22,6 +23,7 @@
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/manifest_handlers/externally_connectable.h"
 #include "extensions/renderer/extension_frame_helper.h"
+#include "extensions/renderer/extension_port.h"
 #include "extensions/renderer/gc_callback.h"
 #include "extensions/renderer/script_context.h"
 #include "extensions/renderer/script_context_set.h"
@@ -50,40 +52,66 @@ using v8_helpers::IsEmptyOrUndefied;
 
 namespace {
 
-void HasMessagePort(int port_id,
+// A simple wrapper to run a given closure when the object goes out of scope,
+// unless reset.
+class RunOnDestruction {

[lazyboy, 2016/09/08 00:05:40]

Can we use ScopedClosureRunner instead?

[Devlin, 2016/09/08 19:15:30]

That exists?  I should have known there are no more original ideas for handy
utilities in chromium. ;)  Done.  I slightly miss the presence of a Reset() that
doesn't warn about the release, but ah well.

+ public:
+  RunOnDestruction(const base::Closure& closure) : closure_(closure) {}
+  ~RunOnDestruction() {
+    if (!closure_.is_null())
+      closure_.Run();
+  }
+
+  void Reset() { closure_.Reset(); }
+
+ private:
+  base::Closure closure_;
+
+  DISALLOW_COPY_AND_ASSIGN(RunOnDestruction);
+};
+
+// A global map between ScriptContext and MessagingBindings.
+base::LazyInstance<std::map<ScriptContext*, MessagingBindings*>>
+    g_messaging_map = LAZY_INSTANCE_INITIALIZER;
+
+void HasMessagePort(int global_port_id,
                     bool* has_port,
                     ScriptContext* script_context) {
   if (*has_port)
     return;  // Stop checking if the port was found.
 
-  v8::Isolate* isolate = script_context->isolate();
-  v8::HandleScope handle_scope(isolate);
-
-  v8::Local<v8::Value> port_id_handle = v8::Integer::New(isolate, port_id);
-  v8::Local<v8::Value> v8_has_port =
-      script_context->module_system()->CallModuleMethod("messaging", "hasPort",
-                                                        1, &port_id_handle);
-  if (IsEmptyOrUndefied(v8_has_port))
-    return;
-  CHECK(v8_has_port->IsBoolean());
-  if (!v8_has_port.As<v8::Boolean>()->Value())
-    return;
-  *has_port = true;
+  MessagingBindings* bindings = g_messaging_map.Get()[script_context];
+  DCHECK(bindings);
+  if (bindings->GetPortWithGlobalId(global_port_id))
+    *has_port = true;
 }
 
 void DispatchOnConnectToScriptContext(
-    int target_port_id,
+    int global_target_port_id,
     const std::string& channel_name,
     const ExtensionMsg_TabConnectionInfo* source,
     const ExtensionMsg_ExternalConnectionInfo& info,
     const std::string& tls_channel_id,
     bool* port_created,

[lazyboy, 2016/09/08 00:05:40]

See comment in related to HasMessagePort, I think we should be able to simplify
this too.

[Devlin, 2016/09/08 19:15:31]

See reply in HasMessagePort() comment. :)

     ScriptContext* script_context) {
+  MessagingBindings* bindings = g_messaging_map.Get()[script_context];
+  DCHECK(bindings);
+
+  int opposite_port_id = global_target_port_id ^ 1;
+  if (bindings->GetPortWithGlobalId(opposite_port_id))
+    return;  // The channel was opened by this same context; ignore it.
+
+  ExtensionPort* port =
+      bindings->CreateNewPortWithGlobalId(global_target_port_id);
+  int local_port_id = port->local_id();
+  // Remove the port
+  RunOnDestruction remove_port(
+      base::Bind(&MessagingBindings::RemovePortWithLocalId,
+                 bindings->GetWeakPtr(), local_port_id));
+
   v8::Isolate* isolate = script_context->isolate();
   v8::HandleScope handle_scope(isolate);
 
-  std::unique_ptr<content::V8ValueConverter> converter(
-      content::V8ValueConverter::create());
 
   const std::string& source_url_spec = info.source_url.spec();
   std::string target_extension_id = script_context->GetExtensionID();
@@ -95,8 +123,11 @@ void DispatchOnConnectToScriptContext(
   v8::Local<v8::Value> guest_render_frame_routing_id = v8::Undefined(isolate);
 
   if (extension) {
-    if (!source->tab.empty() && !extension->is_platform_app())
+    if (!source->tab.empty() && !extension->is_platform_app()) {
+      std::unique_ptr<content::V8ValueConverter> converter(
+          content::V8ValueConverter::create());
       tab = converter->ToV8Value(&source->tab, script_context->v8_context());
+    }
 
     ExternallyConnectableInfo* externally_connectable =
         ExternallyConnectableInfo::Get(extension);
@@ -129,7 +160,7 @@ void DispatchOnConnectToScriptContext(
 
   v8::Local<v8::Value> arguments[] = {
       // portId
-      v8::Integer::New(isolate, target_port_id),
+      v8::Integer::New(isolate, local_port_id),
       // channelName
       v8_channel_name,
       // sourceTab
@@ -156,32 +187,29 @@ void DispatchOnConnectToScriptContext(
 
   if (!IsEmptyOrUndefied(retval)) {
     CHECK(retval->IsBoolean());
-    *port_created |= retval.As<v8::Boolean>()->Value();
+    bool used = retval.As<v8::Boolean>()->Value();
+    *port_created |= used;
+    if (used)
+      remove_port.Reset();  // Port was used; don't remove it.
   } else {
     LOG(ERROR) << "Empty return value from dispatchOnConnect.";
   }
 }
 
 void DeliverMessageToScriptContext(const Message& message,
-                                   int target_port_id,
+                                   int global_target_port_id,
                                    ScriptContext* script_context) {
+  MessagingBindings* bindings = g_messaging_map.Get()[script_context];
+  DCHECK(bindings);
+  ExtensionPort* port = bindings->GetPortWithGlobalId(global_target_port_id);
+  if (!port)
+    return;
+
   v8::Isolate* isolate = script_context->isolate();
   v8::HandleScope handle_scope(isolate);
 
-  // Check to see whether the context has this port before bothering to create
-  // the message.
   v8::Local<v8::Value> port_id_handle =
-      v8::Integer::New(isolate, target_port_id);
-  v8::Local<v8::Value> has_port =
-      script_context->module_system()->CallModuleMethod("messaging", "hasPort",
-                                                        1, &port_id_handle);
-  // Could be empty/undefined if an exception was thrown.
-  // TODO(kalman): Should this be built into CallModuleMethod?
-  if (IsEmptyOrUndefied(has_port))
-    return;
-  CHECK(has_port->IsBoolean());
-  if (!has_port.As<v8::Boolean>()->Value())
-    return;
+      v8::Integer::New(isolate, port->local_id());
 
   v8::Local<v8::String> v8_data;
   if (!ToV8String(isolate, message.data.c_str(), &v8_data))
@@ -207,14 +235,20 @@ void DeliverMessageToScriptContext(const Message& message,
       "messaging", "dispatchOnMessage", &arguments);
 }
 
-void DispatchOnDisconnectToScriptContext(int port_id,
+void DispatchOnDisconnectToScriptContext(int global_port_id,
                                          const std::string& error_message,
                                          ScriptContext* script_context) {
+  MessagingBindings* bindings = g_messaging_map.Get()[script_context];
+  DCHECK(bindings);
+  ExtensionPort* port = bindings->GetPortWithGlobalId(global_port_id);
+  if (!port)
+    return;
+
   v8::Isolate* isolate = script_context->isolate();
   v8::HandleScope handle_scope(isolate);
 
   std::vector<v8::Local<v8::Value>> arguments;
-  arguments.push_back(v8::Integer::New(isolate, port_id));
+  arguments.push_back(v8::Integer::New(isolate, port->local_id()));
   v8::Local<v8::String> v8_error_message;
   if (!error_message.empty())
     ToV8String(isolate, error_message.c_str(), &v8_error_message);
@@ -232,6 +266,7 @@ void DispatchOnDisconnectToScriptContext(int port_id,
 
 MessagingBindings::MessagingBindings(ScriptContext* context)
     : ObjectBackedNativeHandler(context), weak_ptr_factory_(this) {
+  g_messaging_map.Get()[context] = this;
   RouteFunction("CloseChannel", base::Bind(&MessagingBindings::CloseChannel,
                                            base::Unretained(this)));
   RouteFunction("PostMessage", base::Bind(&MessagingBindings::PostMessage,
@@ -239,9 +274,20 @@ MessagingBindings::MessagingBindings(ScriptContext* context)
   // TODO(fsamuel, kalman): Move BindToGC out of messaging natives.
   RouteFunction("BindToGC", base::Bind(&MessagingBindings::BindToGC,
                                        base::Unretained(this)));
+  RouteFunction("OpenChannelToExtension", "runtime.connect",
+                base::Bind(&MessagingBindings::OpenChannelToExtension,
+                           base::Unretained(this)));
+  RouteFunction("OpenChannelToNativeApp", "runtime.connectNative",
+                base::Bind(&MessagingBindings::OpenChannelToNativeApp,
+                           base::Unretained(this)));
+  RouteFunction(
+      "OpenChannelToTab",
+      base::Bind(&MessagingBindings::OpenChannelToTab, base::Unretained(this)));
 }
 
-MessagingBindings::~MessagingBindings() {}
+MessagingBindings::~MessagingBindings() {
+  g_messaging_map.Get().erase(context());
+}
 
 // static
 void MessagingBindings::ValidateMessagePort(
@@ -315,19 +361,44 @@ void MessagingBindings::DispatchOnDisconnect(
       base::Bind(&DispatchOnDisconnectToScriptContext, port_id, error_message));
 }
 
+ExtensionPort* MessagingBindings::GetPortWithGlobalId(int id) {
+  for (const auto& key_value : ports_) {
+    if (key_value.second->global_id() == id)
+      return key_value.second.get();
+  }
+  return nullptr;
+}
+
+ExtensionPort* MessagingBindings::CreateNewPortWithGlobalId(int global_id) {
+  int local_id = next_local_id_++;
+  ExtensionPort* port =
+      ports_
+          .insert(std::make_pair(
+              local_id, base::MakeUnique<ExtensionPort>(context(), local_id)))
+          .first->second.get();
+  port->SetGlobalId(global_id);
+  return port;
+}
+
+void MessagingBindings::RemovePortWithLocalId(int local_id) {
+  ports_.erase(local_id);
+}
+
+base::WeakPtr<MessagingBindings> MessagingBindings::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 void MessagingBindings::PostMessage(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   // Arguments are (int32_t port_id, string message).
   CHECK(args.Length() == 2 && args[0]->IsInt32() && args[1]->IsString());
 
   int port_id = args[0].As<v8::Int32>()->Value();
-
-  content::RenderFrame* render_frame = context()->GetRenderFrame();
-  if (render_frame) {
-    render_frame->Send(new ExtensionHostMsg_PostMessage(
-        render_frame->GetRoutingID(), port_id,
-        Message(*v8::String::Utf8Value(args[1]),
-                blink::WebUserGestureIndicator::isProcessingUserGesture())));
+  auto iter = ports_.find(port_id);
+  if (iter != ports_.end()) {
+    iter->second->PostExtensionMessage(base::MakeUnique<Message>(
+        *v8::String::Utf8Value(args[1]),
+        blink::WebUserGestureIndicator::isProcessingUserGesture()));
   }
 }
 
@@ -364,14 +435,141 @@ void MessagingBindings::BindToGC(
                  args[1].As<v8::Function>(), fallback);
 }
 
+void MessagingBindings::OpenChannelToExtension(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  content::RenderFrame* render_frame = context()->GetRenderFrame();
+  if (!render_frame)
+    return;
+
+  // The Javascript code should validate/fill the arguments.
+  CHECK_EQ(args.Length(), 3);
+  CHECK(args[0]->IsString());
+  CHECK(args[1]->IsString());
+  CHECK(args[2]->IsBoolean());
+
+  int local_id = next_local_id_++;

[lazyboy, 2016/09/08 00:05:40]

I'd wrap this in GetNextLocalId() { return next_local_id_++ } since this is
called from many places.

[Devlin, 2016/09/08 19:15:31]

Done.

+  ports_[local_id] = base::MakeUnique<ExtensionPort>(context(), local_id);
+
+  ExtensionMsg_ExternalConnectionInfo info;
+  // For messaging APIs, hosted apps should be considered a web page so hide
+  // its extension ID.
+  const Extension* extension = context()->extension();
+  if (extension && !extension->is_hosted_app())
+    info.source_id = extension->id();
+
+  info.target_id = *v8::String::Utf8Value(args[0]);
+  info.source_url = context()->url();
+  std::string channel_name = *v8::String::Utf8Value(args[1]);
+  bool include_tls_channel_id =
+      args.Length() > 2 ? args[2]->BooleanValue() : false;
+
+  ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
+  DCHECK(frame_helper);
+  frame_helper->RequestPortId(
+      info, channel_name, include_tls_channel_id,

[lazyboy, 2016/09/08 00:05:40]

generic: Why is |include_tls_channel_id| not part of |info|? 

[Devlin, 2016/09/08 19:15:30]

I really don't know.  Added a TODO.

+      base::Bind(&MessagingBindings::SetGlobalPortId,
+                 weak_ptr_factory_.GetWeakPtr(), local_id));
+
+  args.GetReturnValue().Set(static_cast<int32_t>(local_id));

[lazyboy, 2016/09/08 00:05:40]

Fingers crossed, hope no one is relying on the old global behavior.

[Devlin, 2016/09/08 19:15:30]

Port ids are an implementation detail, exposed only the pseudo-private PortImpl
JS object (public documentation [1] doesn't include it).  And it seems like all
our tests pass.  If anyone else was relying on somehow snagging the private
field, then I think we're okay breaking them.  Of course, there's the
possibility that we're relying on it somewhere that isn't tested, but I'm hoping
that's not the case.

[1] https://developer.chrome.com/extensions/runtime#type-Port

+}
+
+void MessagingBindings::OpenChannelToNativeApp(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  // The Javascript code should validate/fill the arguments.
+  CHECK_EQ(args.Length(), 1);
+  CHECK(args[0]->IsString());
+  CHECK(context()->GetAvailability("runtime.connectNative").is_available());
+
+  content::RenderFrame* render_frame = context()->GetRenderFrame();
+  if (!render_frame)
+    return;
+
+  std::string native_app_name = *v8::String::Utf8Value(args[0]);
+
+  int local_id = next_local_id_++;
+  ExtensionPort* port =
+      ports_
+          .insert(std::make_pair(
+              local_id, base::MakeUnique<ExtensionPort>(context(), local_id)))
+          .first->second.get();
+
+  int global_id = -1;
+  // TODO(devlin): Make this async.

[lazyboy, 2016/09/08 00:05:40]

Bug ID (here and below)

[Devlin, 2016/09/08 19:15:30]

Done.

+  render_frame->Send(new ExtensionHostMsg_OpenChannelToNativeApp(
+      render_frame->GetRoutingID(), native_app_name, &global_id));
+  port->SetGlobalId(global_id);
+  args.GetReturnValue().Set(static_cast<int32_t>(local_id));
+}
+
+void MessagingBindings::OpenChannelToTab(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  content::RenderFrame* render_frame = context()->GetRenderFrame();
+  if (!render_frame)
+    return;
+
+  // tabs_custom_bindings.js unwraps arguments to tabs.connect/sendMessage and
+  // passes them to OpenChannelToTab, in the following order:
+  // - |tab_id| - Positive number that specifies the destination of the channel.
+  // - |frame_id| - Target frame(s) in the tab where onConnect is dispatched:
+  //   -1 for all frames, 0 for the main frame, >0 for a child frame.
+  // - |extension_id| - Extension ID of sender and destination.

[lazyboy, 2016/09/08 00:05:40]

I find "sender and destination" a bit confusing.

[Devlin, 2016/09/08 19:15:30]

Me too (this was copy-pasted). Replaced with "id of the initiating extension."

+  // - |channel_name| - A user-defined channel name.
+  CHECK(args.Length() >= 4 && args[0]->IsInt32() && args[1]->IsInt32() &&

[lazyboy, 2016/09/08 00:05:40]

Curious: why is this >= 4 and not == 4?

[Devlin, 2016/09/08 19:15:30]

Shouldn't be (copy-paste).  Updated to be == and also to have separate CHECK
statements so we get useful output from any crashes.

+        args[2]->IsString() && args[3]->IsString());
+
+  int local_id = next_local_id_++;
+  ExtensionPort* port =
+      ports_
+          .insert(std::make_pair(
+              local_id, base::MakeUnique<ExtensionPort>(context(), local_id)))
+          .first->second.get();
+
+  ExtensionMsg_TabTargetConnectionInfo info;
+  info.tab_id = args[0]->Int32Value();
+  info.frame_id = args[1]->Int32Value();
+  std::string extension_id = *v8::String::Utf8Value(args[2]);
+  std::string channel_name = *v8::String::Utf8Value(args[3]);
+  int global_id = -1;
+  // TODO(devlin): Make this async.
+  render_frame->Send(new ExtensionHostMsg_OpenChannelToTab(
+      render_frame->GetRoutingID(), info, extension_id, channel_name,
+      &global_id));
+  port->SetGlobalId(global_id);
+  args.GetReturnValue().Set(static_cast<int32_t>(local_id));
+}
+
 void MessagingBindings::ClosePort(int port_id, bool force_close) {

[lazyboy, 2016/09/08 00:05:40]

s/port_id/local_port_id

[Devlin, 2016/09/08 19:15:30]

Done.

   // TODO(robwu): Merge this logic with CloseChannel once the TODO in BindToGC
   // has been addressed.
-  content::RenderFrame* render_frame = context()->GetRenderFrame();
-  if (render_frame) {
-    render_frame->Send(new ExtensionHostMsg_CloseMessagePort(
-        render_frame->GetRoutingID(), port_id, force_close));
+  auto iter = ports_.find(port_id);
+  if (iter != ports_.end()) {
+    std::unique_ptr<ExtensionPort> port = std::move(iter->second);
+    ports_.erase(iter);
+    port->ClosePort(force_close);
+    // If the port hasn't been initialized, we can't delete it just yet, because
+    // we need to wait for it to send any pending messages. This is important
+    // to support the flow:
+    //  var port = chrome.runtime.connect();
+    //  port.postMessage({message});
+    //  port.disconnect();
+    if (!port->initialized())
+      disconnected_ports_[port->local_id()] = std::move(port);
   }
 }
 
+void MessagingBindings::SetGlobalPortId(int local_id, int global_id) {
+  auto iter = ports_.find(local_id);
+  if (iter != ports_.end()) {
+    iter->second->SetGlobalId(global_id);
+    return;
+  }
+
+  iter = disconnected_ports_.find(local_id);
+  DCHECK(iter != disconnected_ports_.end());
+  iter->second->SetGlobalId(global_id);
+  // Setting the global id dispatches pending messages, so we can delete the
+  // port now.
+  disconnected_ports_.erase(iter);
+}
+
 }  // namespace extensions