[Extensions] Make port creation synchronous in unload handlers

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 5462e373229570e937a4b95a9b87d92e86d718ec..61aeb8e5cc326c68159bd7261e844d96ff84921e 100644
--- a/extensions/renderer/messaging_bindings.cc
+++ b/extensions/renderer/messaging_bindings.cc
@@ -269,6 +269,18 @@ MessagingBindings::MessagingBindings(ScriptContext* context)
 
 MessagingBindings::~MessagingBindings() {
   g_messaging_map.Get().erase(context());
+  if (ports_created_normal_ > 0 || ports_created_in_before_unload_ > 0 ||
+      ports_created_in_unload_ > 0) {

[Ilya Sherman, 2016/11/14 18:00:28]

I don't really understand this check -- why do you record the metric iff at
least one of the counts is non-zero?

[Devlin, 2016/11/14 18:50:33]

I think the vast majority of contexts will never create any message ports (all
web pages that don't talk to exxtensions or have an extension running on them,
in addition to any extension contexts that just don't happen to).  As such, I'm
more interested in the relationship between the creation times when ports are
used at all.  We could filter out all the noise of having 90% of the reported
values always be 0, but it seems more targeted to do it like this.  WDYT?

[Ilya Sherman, 2016/11/14 19:00:44]

It just seems like you'll have a weird bias where 0 values might be confusing to
interpret.  But, if you're comfortable with this data format, and taking the
missing zeros into account when analyzing the data, then it seems fine.

+    UMA_HISTOGRAM_COUNTS_100(
+        "Extensions.Messaging.ExtensionPortsCreated.InBeforeUnload",
+        ports_created_in_before_unload_);
+    UMA_HISTOGRAM_COUNTS_100(
+        "Extensions.Messaging.ExtensionPortsCreated.InUnload",
+        ports_created_in_unload_);
+    UMA_HISTOGRAM_COUNTS_100(
+        "Extensions.Messaging.ExtensionPortsCreated.Normal",
+        ports_created_normal_);

[Ilya Sherman, 2016/11/14 18:00:28]

Optional: FWIW, it would probably be more memory efficient, and more or less
equally performant, to use UMA_HISTOGRAM_SPARSE_SLOWLY here, with some code to
forcibly cap the value that's reported.

[Devlin, 2016/11/14 18:50:33]

As discussed offline, actually going for UMA_HISTOGRAM_COUNTS_1000 here.

+  }
 }
 
 // static
@@ -450,10 +462,30 @@ void MessagingBindings::OpenChannelToExtension(
 
   ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
   DCHECK(frame_helper);
-  frame_helper->RequestPortId(
-      info, channel_name, include_tls_channel_id,
-      base::Bind(&MessagingBindings::SetGlobalPortId,
-                 weak_ptr_factory_.GetWeakPtr(), local_id));
+
+  blink::WebLocalFrame* web_frame = render_frame->GetWebFrame();
+  // If the frame is unloading, we need to assign the port id synchronously to
+  // avoid dropping the message on the floor; see crbug.com/660706.
+  // TODO(devlin): Investigate whether we need to continue supporting this long-
+  // term, and, if so, find an alternative that doesn't require synchronous
+  // IPCs.
+  if (!web_frame->document().isNull() &&
+      (web_frame->document().unloadStartedDoNotUse() ||
+       web_frame->document().processingBeforeUnloadDoNotUse())) {
+    ports_created_in_before_unload_ +=
+        web_frame->document().processingBeforeUnloadDoNotUse() ? 1 : 0;
+    ports_created_in_unload_ +=
+        web_frame->document().unloadStartedDoNotUse() ? 1 : 0;
+    int global_id = frame_helper->RequestSyncPortId(info, channel_name,
+                                                    include_tls_channel_id);
+    ports_[local_id]->SetGlobalId(global_id);
+  } else {
+    ++ports_created_normal_;
+    frame_helper->RequestPortId(
+        info, channel_name, include_tls_channel_id,
+        base::Bind(&MessagingBindings::SetGlobalPortId,
+                   weak_ptr_factory_.GetWeakPtr(), local_id));
+  }
 
   args.GetReturnValue().Set(static_cast<int32_t>(local_id));
 }