Fix telemetry hangs with PlzNavigate.

content/browser/devtools/render_frame_devtools_agent_host.cc

Index: content/browser/devtools/render_frame_devtools_agent_host.cc
diff --git a/content/browser/devtools/render_frame_devtools_agent_host.cc b/content/browser/devtools/render_frame_devtools_agent_host.cc
index 76b3866d638974a579b444ec8e8ee4eafc6b3e96..3f3baf1467b295cc39570edba3bf54241b6b01d0 100644
--- a/content/browser/devtools/render_frame_devtools_agent_host.cc
+++ b/content/browser/devtools/render_frame_devtools_agent_host.cc
@@ -130,6 +130,8 @@ class RenderFrameDevToolsAgentHost::FrameHostHolder {
   std::vector<std::pair<int, std::string>> pending_messages_;
   // <call_id> -> PendingMessage
   std::map<int, PendingMessage> sent_messages_;
+  // These are sent messages for which we got a reply while suspended.
+  std::map<int, PendingMessage> sent_messages_whose_reply_came_while_suspended_;
 };
 
 RenderFrameDevToolsAgentHost::FrameHostHolder::FrameHostHolder(
@@ -165,6 +167,13 @@ void RenderFrameDevToolsAgentHost::FrameHostHolder::Reattach(
       host_->GetRoutingID(), agent_->GetId(), agent_->session()->session_id(),
       chunk_processor_.state_cookie()));
   if (old) {
+    if (IsBrowserSideNavigationEnabled()) {
+      for (const auto& pair :
+               old->sent_messages_whose_reply_came_while_suspended_) {
+        DispatchProtocolMessage(pair.second.session_id, pair.first,
+                                pair.second.method, pair.second.message);
+      }
+    }
     for (const auto& pair : old->sent_messages_) {
       DispatchProtocolMessage(pair.second.session_id, pair.first,
                               pair.second.method, pair.second.message);
@@ -233,11 +242,14 @@ RenderFrameDevToolsAgentHost::FrameHostHolder::ProcessChunkedMessageFromAgent(
 void RenderFrameDevToolsAgentHost::FrameHostHolder::SendMessageToClient(
     int session_id,
     const std::string& message) {
-  sent_messages_.erase(chunk_processor_.last_call_id());
-  if (suspended_)
+  int id = chunk_processor_.last_call_id();
+  if (suspended_) {
+    sent_messages_whose_reply_came_while_suspended_[id] = sent_messages_[id];
     pending_messages_.push_back(std::make_pair(session_id, message));
-  else
+  } else {
     agent_->SendMessageToClient(session_id, message);
+  }
+  sent_messages_.erase(id);
 }
 
 void RenderFrameDevToolsAgentHost::FrameHostHolder::Suspend() {
@@ -250,6 +262,7 @@ void RenderFrameDevToolsAgentHost::FrameHostHolder::Resume() {
     agent_->SendMessageToClient(pair.first, pair.second);
   std::vector<std::pair<int, std::string>> empty;
   pending_messages_.swap(empty);
+  sent_messages_whose_reply_came_while_suspended_.clear();
 }
 
 // RenderFrameDevToolsAgentHost ------------------------------------------------
@@ -691,6 +704,8 @@ void RenderFrameDevToolsAgentHost::DidFinishNavigation(
       DiscardPending();
     }
     pending_handle_ = nullptr;
+  } else {
+    current_->Resume();

[dgozman, 2016/12/19 18:26:44]

Let's only resume when navigation_handles_ is empty to be on the safe side.

[jam, 2016/12/19 20:13:02]

Ok, I don't really understand why there are multiple NavigationHandles being
tracked? 

[dgozman, 2016/12/19 20:29:52]

I'm not sure. From the time this was added, I vaguely remember there could be
multiple navigations at the same time (maybe in download case or something?) Or
this is just wrong.

[nasko, 2016/12/19 20:59:00]

Yes, you can have multiple navigations simultaneously, though rarely. The most
common example is a cross-process navigation that takes long time and a
pushState/replaceState/fragment navigation in the meantime. The latter is
considered "same page", so it starts/commits/finishes in a single event loop and
the cross-process slow navigation continues to exist until timeout or commit.

[jam, 2016/12/19 22:23:15]

Thanks, this is helpful to know. You say "most common example", what are the
other cases? If that was the only example, I would have changed this code to
ignore in-page navigations and just have one NavigationHandle instead of a set
which would simplify things.

   }
   DispatchBufferedProtocolMessagesIfNecessary();
 
@@ -732,6 +747,7 @@ void RenderFrameDevToolsAgentHost::AboutToNavigate(
     return;
   DCHECK(current_);
   navigating_handles_.insert(navigation_handle);
+  current_->Suspend();

[dgozman, 2016/12/19 18:26:44]

I think we should only do this (and line before) if navigation_handle's
FrameTreeNode equals frame_tree_node_. Otherwise, we'll suspend for any subframe
navigation which is unnecessary.

[jam, 2016/12/19 20:13:02]

That's always the case, i.e. see 
https://cs.chromium.org/chromium/src/content/browser/devtools/render_frame_de...

The static RenderFrameDevToolsAgentHost::OnBeforeNavigation(NavigationHandle*
navigation_handle) method always finds the right RenderFrameDevToolsAgentHost by
comparing frametreenodes.

[dgozman, 2016/12/19 20:29:52]

Right, my bad.

   DCHECK(CheckConsistency());
 }