[Extensions] Finish making port creation asynchronous

extensions/renderer/messaging_bindings.cc

 // Copyright 2014 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 "extensions/renderer/messaging_bindings.h"
 
 #include <stdint.h>
 
 #include <map>
 #include <string>
@@ skipping 455 matching lines @@
   // This should be checked by our function routing code.
   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 = 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);
 
-  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));
-  }
-
-  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));
 }
 
 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| - ID of the initiating extension.
   // - |channel_name| - A user-defined channel name.
   CHECK(args.Length() == 4);
   CHECK(args[0]->IsInt32());
   CHECK(args[1]->IsInt32());
   CHECK(args[2]->IsString());
   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));
 }
 
 void MessagingBindings::ClosePort(int local_port_id, bool force_close) {
   // TODO(robwu): Merge this logic with CloseChannel once the TODO in BindToGC
   // has been addressed.
   auto iter = ports_.find(local_port_id);
   if (iter != ports_.end()) {
     std::unique_ptr<ExtensionPort> port = std::move(iter->second);
     ports_.erase(iter);
@@ skipping 20 matching lines @@
   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);
 }
 
 int MessagingBindings::GetNextLocalId() { return next_local_id_++; }
 
 }  // namespace extensions