[Extensions] Finish making port creation 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 6f6f938ed4352c8c5b69fc24f38c86676911ea10..fa1c08ba2b8b16286f8ce7a421aa8638d1fa430f 100644
--- a/extensions/renderer/messaging_bindings.cc
+++ b/extensions/renderer/messaging_bindings.cc
@@ -473,22 +473,14 @@ void MessagingBindings::OpenChannelToNativeApp(
   std::string native_app_name = *v8::String::Utf8Value(args[0]);
 
   int local_id = GetNextLocalId();
-  ExtensionPort* port =
-      ports_
-          .insert(std::make_pair(
-              local_id, base::MakeUnique<ExtensionPort>(context(), local_id)))
-          .first->second.get();
-
-  int global_id = -1;
-  {
-    SCOPED_UMA_HISTOGRAM_TIMER(
-        "Extensions.Messaging.GetPortIdSyncTime.NativeApp");
-    // TODO(devlin): Make this async. crbug.com/642380
-    render_frame->Send(new ExtensionHostMsg_OpenChannelToNativeApp(
-        render_frame->GetRoutingID(), native_app_name, &global_id));
-  }
+  ports_[local_id] = base::MakeUnique<ExtensionPort>(context(), local_id);
 
-  port->SetGlobalId(global_id);
+  ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
+  DCHECK(frame_helper);

[lazyboy, 2016/09/13 00:34:39]

I've found from reviews that this is unncessary, since things will crash on next
line anyway if frame_helper is nullptr.

[Devlin, 2016/09/13 16:49:55]

I don't quite agree.  For one thing, even if it crashes immediately, DCHECK()s
give us nice, readable stack traces (DCHECK failed at line n), whereas segfaults
sometimes just say "segfault", and you're left to determine where exactly it
happened.  Additionally, depending on how the compiler generates the code,
accessing a nullptr might not always crash immediately.  For instance, if the
compiler inlines the method, it could then try to read from the necessary
offsets for the variables used (and unlikely, but possibly (?) succeed), and you
can get a crash somewhere deeper in the stack.  Overall, I'd just prefer the
DCHECK unless there's good reason not to.  WDYT?

[lazyboy, 2016/09/13 17:22:08]

In practice, I've found that too while running locally. The review comment
I mentioned was proabably because they have same effect in crash/ logs?
Either way, I'm fine with the DCHECK.

 and you're left to determine where exactly it
Good point.

  For instance, if the

+  frame_helper->RequestNativePortId(
+      native_app_name,
+      base::Bind(&MessagingBindings::SetGlobalPortId,
+                 weak_ptr_factory_.GetWeakPtr(), local_id));
   args.GetReturnValue().Set(static_cast<int32_t>(local_id));
 }
 
@@ -512,26 +504,22 @@ void MessagingBindings::OpenChannelToTab(
   CHECK(args[3]->IsString());
 
   int local_id = GetNextLocalId();
-  ExtensionPort* port =
-      ports_
-          .insert(std::make_pair(
-              local_id, base::MakeUnique<ExtensionPort>(context(), local_id)))
-          .first->second.get();
+  ports_[local_id] = base::MakeUnique<ExtensionPort>(context(), local_id);
 
   ExtensionMsg_TabTargetConnectionInfo info;
   info.tab_id = args[0]->Int32Value();
   info.frame_id = args[1]->Int32Value();
+  // TODO(devlin): Why is this not part of info?
   std::string extension_id = *v8::String::Utf8Value(args[2]);
   std::string channel_name = *v8::String::Utf8Value(args[3]);
-  int global_id = -1;
-  {
-    SCOPED_UMA_HISTOGRAM_TIMER("Extensions.Messaging.GetPortIdSyncTime.Tab");
-    // TODO(devlin): Make this async. crbug.com/642380
-    render_frame->Send(new ExtensionHostMsg_OpenChannelToTab(
-        render_frame->GetRoutingID(), info, extension_id, channel_name,
-        &global_id));
-  }
-  port->SetGlobalId(global_id);
+
+  ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
+  DCHECK(frame_helper);
+  frame_helper->RequestTabPortId(
+      info, extension_id, channel_name,
+      base::Bind(&MessagingBindings::SetGlobalPortId,
+                 weak_ptr_factory_.GetWeakPtr(), local_id));
+
   args.GetReturnValue().Set(static_cast<int32_t>(local_id));
 }