[Extensions] Make port creation synchronous in unload handlers

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 251 matching lines @@
   RouteFunction("OpenChannelToNativeApp", "runtime.connectNative",
                 base::Bind(&MessagingBindings::OpenChannelToNativeApp,
                            base::Unretained(this)));
   RouteFunction(
       "OpenChannelToTab",
       base::Bind(&MessagingBindings::OpenChannelToTab, base::Unretained(this)));
 }
 
 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
 void MessagingBindings::ValidateMessagePort(
     const ScriptContextSet& context_set,
     int global_port_id,
     content::RenderFrame* render_frame) {
   int routing_id = render_frame->GetRoutingID();
 
   bool has_port = false;
@@ skipping 161 matching lines @@
 
   info.target_id = *v8::String::Utf8Value(args[0]);
   info.source_url = context()->url();
   std::string channel_name = *v8::String::Utf8Value(args[1]);
   // TODO(devlin): Why is this not part of info?
   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,
-      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));
 }
 
 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());
   // This should be checked by our function routing code.
@@ skipping 86 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