Fix telemetry hangs with PlzNavigate.

content/browser/devtools/render_frame_devtools_agent_host.cc

 // Copyright (c) 2012 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 "content/browser/devtools/render_frame_devtools_agent_host.h"
 
 #include <tuple>
 #include <utility>
 
 #include "base/guid.h"
@@ skipping 112 matching lines @@
 
   RenderFrameDevToolsAgentHost* agent_;
   RenderFrameHostImpl* host_;
   bool attached_;
   bool suspended_;
   DevToolsMessageChunkProcessor chunk_processor_;
   // <session_id, message>
   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(
     RenderFrameDevToolsAgentHost* agent, RenderFrameHostImpl* host)
     : agent_(agent),
       host_(host),
       attached_(false),
       suspended_(false),
       chunk_processor_(base::Bind(
            &RenderFrameDevToolsAgentHost::FrameHostHolder::SendMessageToClient,
@@ skipping 15 matching lines @@
 }
 
 void RenderFrameDevToolsAgentHost::FrameHostHolder::Reattach(
     FrameHostHolder* old) {
   if (old)
     chunk_processor_.set_state_cookie(old->chunk_processor_.state_cookie());
   host_->Send(new DevToolsAgentMsg_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);
     }
   }
   GrantPolicy();
   attached_ = true;
 }
 
 void RenderFrameDevToolsAgentHost::FrameHostHolder::Detach() {
@@ skipping 48 matching lines @@
 
 bool
 RenderFrameDevToolsAgentHost::FrameHostHolder::ProcessChunkedMessageFromAgent(
     const DevToolsMessageChunk& chunk) {
   return chunk_processor_.ProcessChunkedMessageFromAgent(chunk);
 }
 
 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() {
   suspended_ = true;
 }
 
 void RenderFrameDevToolsAgentHost::FrameHostHolder::Resume() {
   suspended_ = false;
   for (const auto& pair : pending_messages_)
     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 ------------------------------------------------
 
 // static
 scoped_refptr<DevToolsAgentHost>
 DevToolsAgentHost::GetOrCreateFor(RenderFrameHost* frame_host) {
   while (frame_host && !ShouldCreateDevToolsFor(frame_host))
     frame_host = frame_host->GetParent();
   DCHECK(frame_host);
@@ skipping 315 matching lines @@
     return true;
   }
 
   int call_id = 0;
   std::string method;
   if (protocol_handler_->HandleOptionalMessage(
           session()->session_id(), std::move(value), &call_id, &method)) {
     return true;
   }
 
   if (!navigating_handles_.empty()) {

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

I think that bug#2 from description is addressed here, isn't it? Since we now
suspend early, no messages are being sent after AboutToNavigate up to the
DidFinishNavigation - they are put to in_navigation_protocol_message_buffer_.

The fix with sent_messages_whose_reply_came_while_suspended_ addresses the issue
that we get the new state_cookie with a reply, and thus we have to resend all
the messages we didn't get response for before navigation to get the same
backend state in new renderer. WDYT?

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

The Page.navigate message is sent before we're (now) paused. It's step 1 in the
description, while the pausing is now happening in step 2.

     DCHECK(IsBrowserSideNavigationEnabled());
     in_navigation_protocol_message_buffer_[call_id] =
         { session()->session_id(), method, message };
     return true;
   }
 
   if (current_) {
     current_->DispatchProtocolMessage(
         session()->session_id(), call_id, method, message);
   }
@@ skipping 85 matching lines @@
   if (pending_handle_ == navigation_handle) {
     // This navigation handle did set the pending FrameHostHolder.
     DCHECK(pending_);
     if (navigation_handle->HasCommitted()) {
       DCHECK(pending_->host() == navigation_handle->GetRenderFrameHost());
       CommitPending();
     } else {
       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();
 
   DCHECK(CheckConsistency());
   if (target_handler_ && navigation_handle->HasCommitted())
     target_handler_->UpdateServiceWorkers();
 }
 
 void RenderFrameDevToolsAgentHost::AboutToNavigateRenderFrame(
     RenderFrameHost* old_host,
@@ skipping 21 matching lines @@
     CommitPending();
   DCHECK(CheckConsistency());
 }
 
 void RenderFrameDevToolsAgentHost::AboutToNavigate(
     NavigationHandle* navigation_handle) {
   if (!IsBrowserSideNavigationEnabled())
     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());
 }
 
 void RenderFrameDevToolsAgentHost::RenderFrameHostChanged(
     RenderFrameHost* old_host,
     RenderFrameHost* new_host) {
   // CommitPending may destruct |this|.
   scoped_refptr<RenderFrameDevToolsAgentHost> protect(this);
 
   if (target_handler_)
@@ skipping 447 matching lines @@
     RenderFrameHost* host) {
   return (current_ && current_->host() == host) ||
       (pending_ && pending_->host() == host);
 }
 
 bool RenderFrameDevToolsAgentHost::IsChildFrame() {
   return current_ && current_->host()->GetParent();
 }
 
 }  // namespace content