PlzNavigate: fix DevToolsProtocolTest.CrossSitePauseInBeforeUnload

This CL fixes DevToolsProtocolTest.CrossSitePauseInBeforeUnload when run with
PlzNavigate enabled. Now, devtools protocol messages are deferred only after
the browser has received the BeforeUnload ACK (and not before).

BUG=551000
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review URL: https://codereview.chromium.org/1729373002

Cr-Commit-Position: refs/heads/master@{#378741}

5 files changed, 82 insertions, 64 deletions

diff --git a/content/browser/devtools/protocol/devtools_protocol_browsertest.cc b/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
index 62f7a35..7987e93 100644
--- a/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
+++ b/content/browser/devtools/protocol/devtools_protocol_browsertest.cc
@@ -344,7 +344,9 @@ IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest, NavigationPreservesMessages) {
   scoped_ptr<base::DictionaryValue> params(new base::DictionaryValue());
   test_url = GetTestUrl("devtools", "navigation.html");
   params->SetString("url", test_url.spec());
+  TestNavigationObserver navigation_observer(shell()->web_contents());
   SendCommand("Page.navigate", std::move(params), true);
+  navigation_observer.Wait();
 
   bool enough_results = result_ids_.size() >= 2u;
   EXPECT_TRUE(enough_results);
diff --git a/content/browser/devtools/render_frame_devtools_agent_host.cc b/content/browser/devtools/render_frame_devtools_agent_host.cc
index 53c8ed3..c556075 100644
--- a/content/browser/devtools/render_frame_devtools_agent_host.cc
+++ b/content/browser/devtools/render_frame_devtools_agent_host.cc
@@ -48,8 +48,6 @@ typedef std::vector<RenderFrameDevToolsAgentHost*> Instances;
 namespace {
 base::LazyInstance<Instances>::Leaky g_instances = LAZY_INSTANCE_INITIALIZER;
 
-bool browser_side_navigation = false;
-
 static RenderFrameDevToolsAgentHost* FindAgentHost(RenderFrameHost* host) {
   if (g_instances == NULL)
     return NULL;
@@ -61,6 +59,18 @@ static RenderFrameDevToolsAgentHost* FindAgentHost(RenderFrameHost* host) {
   return NULL;
 }
 
+static RenderFrameDevToolsAgentHost* FindAgentHost(
+    FrameTreeNode* frame_tree_node) {
+  if (g_instances == NULL)
+    return NULL;
+  for (Instances::iterator it = g_instances.Get().begin();
+       it != g_instances.Get().end(); ++it) {
+    if ((*it)->frame_tree_node() == frame_tree_node)
+      return *it;
+  }
+  return NULL;
+}
+
 // Returns RenderFrameDevToolsAgentHost attached to any of RenderFrameHost
 // instances associated with |web_contents|
 static RenderFrameDevToolsAgentHost* FindAgentHost(WebContents* web_contents) {
@@ -312,7 +322,7 @@ void RenderFrameDevToolsAgentHost::AddAllAgentHosts(
 void RenderFrameDevToolsAgentHost::OnCancelPendingNavigation(
     RenderFrameHost* pending,
     RenderFrameHost* current) {
-  if (browser_side_navigation)
+  if (IsBrowserSideNavigationEnabled())
     return;
 
   RenderFrameDevToolsAgentHost* agent_host = FindAgentHost(pending);
@@ -332,6 +342,16 @@ void RenderFrameDevToolsAgentHost::OnBeforeNavigation(
     agent_host->AboutToNavigateRenderFrame(current, pending);
 }
 
+// static
+void RenderFrameDevToolsAgentHost::OnBeforeNavigation(
+    NavigationHandle* navigation_handle) {
+  FrameTreeNode* frame_tree_node =
+      static_cast<NavigationHandleImpl*>(navigation_handle)->frame_tree_node();
+  RenderFrameDevToolsAgentHost* agent_host = FindAgentHost(frame_tree_node);
+  if (agent_host)
+    agent_host->AboutToNavigate(navigation_handle);
+}
+
 RenderFrameDevToolsAgentHost::RenderFrameDevToolsAgentHost(
     RenderFrameHostImpl* host)
     : dom_handler_(new devtools::dom::DOMHandler()),
@@ -352,9 +372,7 @@ RenderFrameDevToolsAgentHost::RenderFrameDevToolsAgentHost(
       protocol_handler_(new DevToolsProtocolHandler(this)),
       current_frame_crashed_(false),
       pending_handle_(nullptr),
-      in_navigation_(0),
       frame_tree_node_(host->frame_tree_node()) {
-  browser_side_navigation = IsBrowserSideNavigationEnabled();
   DevToolsProtocolDispatcher* dispatcher = protocol_handler_->dispatcher();
   dispatcher->SetDOMHandler(dom_handler_.get());
   dispatcher->SetInputHandler(input_handler_.get());
@@ -456,8 +474,8 @@ bool RenderFrameDevToolsAgentHost::DispatchProtocolMessage(
   if (protocol_handler_->HandleOptionalMessage(session_id(), message, &call_id))
     return true;
 
-  if (in_navigation_ > 0) {
-    DCHECK(browser_side_navigation);
+  if (!navigating_handles_.empty()) {
+    DCHECK(IsBrowserSideNavigationEnabled());
     in_navigation_protocol_message_buffer_[call_id] =
         std::make_pair(session_id(), message);
     return true;
@@ -521,54 +539,49 @@ RenderFrameDevToolsAgentHost::~RenderFrameDevToolsAgentHost() {
     g_instances.Get().erase(it);
 }
 
-void RenderFrameDevToolsAgentHost::DidStartNavigation(
-    NavigationHandle* navigation_handle) {
-  if (!browser_side_navigation)
-    return;
-  if (!MatchesMyTreeNode(navigation_handle))
-    return;
-  DCHECK(current_);
-  DCHECK(in_navigation_ >= 0);
-  ++in_navigation_;
-}
-
 void RenderFrameDevToolsAgentHost::ReadyToCommitNavigation(
     NavigationHandle* navigation_handle) {
   // ReadyToCommitNavigation should only be called in PlzNavigate.
-  DCHECK(browser_side_navigation);
-
-  if (MatchesMyTreeNode(navigation_handle) && in_navigation_ != 0) {
-    RenderFrameHostImpl* render_frame_host_impl =
-        static_cast<RenderFrameHostImpl*>(
-            navigation_handle->GetRenderFrameHost());
-    if (current_->host() != render_frame_host_impl || current_frame_crashed_) {
-      SetPending(render_frame_host_impl);
-      pending_handle_ = navigation_handle;
-    }
+  DCHECK(IsBrowserSideNavigationEnabled());
+
+  // If the navigation is not tracked, return;
+  if (navigating_handles_.count(navigation_handle) == 0)
+    return;
+
+  RenderFrameHostImpl* render_frame_host_impl =
+      static_cast<RenderFrameHostImpl*>(
+          navigation_handle->GetRenderFrameHost());
+  if (current_->host() != render_frame_host_impl || current_frame_crashed_) {
+    SetPending(render_frame_host_impl);
+    pending_handle_ = navigation_handle;
   }
 }
 
 void RenderFrameDevToolsAgentHost::DidFinishNavigation(
     NavigationHandle* navigation_handle) {
-  if (!browser_side_navigation)
+  if (!IsBrowserSideNavigationEnabled())
+    return;
+
+  // If the navigation is not tracked, return;
+  if (navigating_handles_.count(navigation_handle) == 0)
     return;
 
-  if (MatchesMyTreeNode(navigation_handle) && in_navigation_ != 0) {
-    --in_navigation_;
-    DCHECK(in_navigation_ >= 0);
-    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;
+  // Now that the navigation is finished, remove the handle from the list of
+  // navigating handles.
+  navigating_handles_.erase(navigation_handle);
+
+  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();
     }
-    DispatchBufferedProtocolMessagesIfNecessary();
+    pending_handle_ = nullptr;
   }
+  DispatchBufferedProtocolMessagesIfNecessary();
 
   if (navigation_handle->HasCommitted())
     service_worker_handler_->UpdateHosts();
@@ -577,7 +590,7 @@ void RenderFrameDevToolsAgentHost::DidFinishNavigation(
 void RenderFrameDevToolsAgentHost::AboutToNavigateRenderFrame(
     RenderFrameHost* old_host,
     RenderFrameHost* new_host) {
-  if (browser_side_navigation)
+  if (IsBrowserSideNavigationEnabled())
     return;
 
   DCHECK(!pending_ || pending_->host() != old_host);
@@ -589,10 +602,18 @@ void RenderFrameDevToolsAgentHost::AboutToNavigateRenderFrame(
   SetPending(static_cast<RenderFrameHostImpl*>(new_host));
 }
 
+void RenderFrameDevToolsAgentHost::AboutToNavigate(
+    NavigationHandle* navigation_handle) {
+  if (!IsBrowserSideNavigationEnabled())
+    return;
+  DCHECK(current_);
+  navigating_handles_.insert(navigation_handle);
+}
+
 void RenderFrameDevToolsAgentHost::RenderFrameHostChanged(
     RenderFrameHost* old_host,
     RenderFrameHost* new_host) {
-  if (browser_side_navigation)
+  if (IsBrowserSideNavigationEnabled())
     return;
 
   DCHECK(!pending_ || pending_->host() != old_host);
@@ -609,7 +630,7 @@ void RenderFrameDevToolsAgentHost::RenderFrameHostChanged(
 
 void RenderFrameDevToolsAgentHost::FrameDeleted(RenderFrameHost* rfh) {
   if (pending_ && pending_->host() == rfh) {
-    if (!browser_side_navigation)
+    if (!IsBrowserSideNavigationEnabled())
       DiscardPending();
     return;
   }
@@ -712,7 +733,7 @@ void RenderFrameDevToolsAgentHost::DidCommitProvisionalLoadForFrame(
     RenderFrameHost* render_frame_host,
     const GURL& url,
     ui::PageTransition transition_type) {
-  if (browser_side_navigation)
+  if (IsBrowserSideNavigationEnabled())
     return;
   if (pending_ && pending_->host() == render_frame_host)
     CommitPending();
@@ -725,7 +746,7 @@ void RenderFrameDevToolsAgentHost::DidFailProvisionalLoad(
     int error_code,
     const base::string16& error_description,
     bool was_ignored_by_handler) {
-  if (browser_side_navigation)
+  if (IsBrowserSideNavigationEnabled())
     return;
   if (pending_ && pending_->host() == render_frame_host)
     DiscardPending();
@@ -733,7 +754,8 @@ void RenderFrameDevToolsAgentHost::DidFailProvisionalLoad(
 
 void RenderFrameDevToolsAgentHost::
     DispatchBufferedProtocolMessagesIfNecessary() {
-  if (in_navigation_ == 0 && in_navigation_protocol_message_buffer_.size()) {
+  if (navigating_handles_.empty() &&
+      in_navigation_protocol_message_buffer_.size()) {
     DCHECK(current_);
     for (const auto& pair : in_navigation_protocol_message_buffer_) {
       current_->DispatchProtocolMessage(pair.second.first, pair.first,
@@ -767,7 +789,7 @@ void RenderFrameDevToolsAgentHost::DisconnectWebContents() {
   disconnected_->Detach();
   frame_tree_node_ = nullptr;
   in_navigation_protocol_message_buffer_.clear();
-  in_navigation_ = 0;
+  navigating_handles_.clear();
   pending_handle_ = nullptr;
   WebContentsObserver::Observe(nullptr);
 }
@@ -892,11 +914,4 @@ bool RenderFrameDevToolsAgentHost::IsChildFrame() {
   return current_ && current_->host()->GetParent();
 }
 
-bool RenderFrameDevToolsAgentHost::MatchesMyTreeNode(
-    NavigationHandle* navigation_handle) {
-  return frame_tree_node_ ==
-         static_cast<NavigationHandleImpl*>(navigation_handle)
-             ->frame_tree_node();
-}
-
 }  // namespace content
diff --git a/content/browser/devtools/render_frame_devtools_agent_host.h b/content/browser/devtools/render_frame_devtools_agent_host.h
index f190d4c..24d0535 100644
--- a/content/browser/devtools/render_frame_devtools_agent_host.h
+++ b/content/browser/devtools/render_frame_devtools_agent_host.h
@@ -55,12 +55,15 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
                                         RenderFrameHost* current);
   static void OnBeforeNavigation(RenderFrameHost* current,
                                  RenderFrameHost* pending);
+  static void OnBeforeNavigation(NavigationHandle* navigation_handle);
 
   void SynchronousSwapCompositorFrame(
       const cc::CompositorFrameMetadata& frame_metadata);
 
   bool HasRenderFrameHost(RenderFrameHost* host);
 
+  FrameTreeNode* frame_tree_node() { return frame_tree_node_; }
+
   // DevTooolsAgentHost overrides.
   void DisconnectWebContents() override;
   void ConnectWebContents(WebContents* web_contents) override;
@@ -90,7 +93,6 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
   void InspectElement(int x, int y) override;
 
   // WebContentsObserver overrides.
-  void DidStartNavigation(NavigationHandle* navigation_handle) override;
   void ReadyToCommitNavigation(NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(NavigationHandle* navigation_handle) override;
   void RenderFrameHostChanged(RenderFrameHost* old_host,
@@ -116,6 +118,7 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
 
   void AboutToNavigateRenderFrame(RenderFrameHost* old_host,
                                   RenderFrameHost* new_host);
+  void AboutToNavigate(NavigationHandle* navigation_handle);
 
   void DispatchBufferedProtocolMessagesIfNecessary();
 
@@ -137,8 +140,6 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
   void OnRequestNewWindow(RenderFrameHost* sender, int new_routing_id);
   void DestroyOnRenderFrameGone();
 
-  bool MatchesMyTreeNode(NavigationHandle* navigation_handle);
-
   class FrameHostHolder;
 
   scoped_ptr<FrameHostHolder> current_;
@@ -170,9 +171,8 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
   // Handle that caused the setting of pending_.
   NavigationHandle* pending_handle_;
 
-  // Navigation counter and queue for buffering protocol messages during a
-  // navigation.
-  int in_navigation_;
+  // List of handles currently navigating.
+  std::set<NavigationHandle*> navigating_handles_;
 
   // <call_id> -> <session_id, message>
   std::map<int, std::pair<int, std::string>>
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index a0e1e625..83f6f85 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "content/browser/devtools/render_frame_devtools_agent_host.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
@@ -203,6 +204,7 @@ void NavigationRequest::BeginNavigation() {
   DCHECK(!loader_);
   DCHECK(state_ == NOT_STARTED || state_ == WAITING_FOR_RENDERER_RESPONSE);
   state_ = STARTED;
+  RenderFrameDevToolsAgentHost::OnBeforeNavigation(navigation_handle_.get());
 
   if (ShouldMakeNetworkRequestForURL(common_params_.url)) {
     // It's safe to use base::Unretained because this NavigationRequest owns
diff --git a/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter b/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
index 7f31abb..f8c473b 100644
--- a/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
+++ b/testing/buildbot/filters/browser-side-navigation.linux.content_browsertests.filter
@@ -2,7 +2,6 @@
 -CrossSiteTransferTest.NoLeakOnCrossSiteCancel
 -CrossSiteTransferTest.ReplaceEntryCrossProcessThenTransfer
 -CrossSiteTransferTest.ReplaceEntryCrossProcessTwice
--DevToolsProtocolTest.CrossSitePauseInBeforeUnload
 -LoFiResourceDispatcherHostBrowserTest.ShouldEnableLoFiModeNavigateBackThenForward
 -NavigationControllerBrowserTest.FrameNavigationEntry_SubframeHistoryFallback
 -NavigationControllerBrowserTest.StopCausesFailureDespiteJavaScriptURL