Navigation: Move commit_timeout from NavigationHandleImpl.

This includes moving all related functions and members.

This has a potential of reproducing this bug: https://crbug.com/957212.

The bug was initially produced after a big CL was landed that included
some changes from this CL. But due to the size of the changes, it was
hard to point out what the actual problem was.

Bug: 916537
Change-Id: I2c8b11f82e5b5197976b3cdb15de40f3032e68ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1720775
Commit-Queue: Mohamed Abdelhalim <zetamoo@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682680}

content/browser/frame_host/navigation_handle_impl.cc

@@ -31,20 +31,6 @@
 
 namespace content {
 
-namespace {
-
-// Default timeout for the READY_TO_COMMIT -> COMMIT transition.  Chosen
-// initially based on the Navigation.ReadyToCommitUntilCommit UMA, and then
-// refined based on feedback based on CrashExitCodes.Renderer/RESULT_CODE_HUNG.
-constexpr base::TimeDelta kDefaultCommitTimeout =
-    base::TimeDelta::FromSeconds(30);
-
-// Timeout for the READY_TO_COMMIT -> COMMIT transition.
-// Overrideable via SetCommitTimeoutForTesting.
-base::TimeDelta g_commit_timeout = kDefaultCommitTimeout;
-
-}  // namespace
-
 NavigationHandleImpl::NavigationHandleImpl(
     NavigationRequest* navigation_request,
     net::HttpRequestHeaders request_headers)
@@ -368,54 +354,4 @@ const net::ProxyServer& NavigationHandleImpl::GetProxyServer() {
   return navigation_request_->proxy_server();
 }
 
-void NavigationHandleImpl::RenderProcessBlockedStateChanged(bool blocked) {
-  if (blocked)
-    StopCommitTimeout();
-  else
-    RestartCommitTimeout();
-}
-
-void NavigationHandleImpl::StopCommitTimeout() {
-  commit_timeout_timer_.Stop();
-  render_process_blocked_state_changed_subscription_.reset();
-  GetRenderFrameHost()->GetRenderWidgetHost()->RendererIsResponsive();
-}
-
-void NavigationHandleImpl::RestartCommitTimeout() {
-  commit_timeout_timer_.Stop();
-  if (state() >= NavigationRequest::DID_COMMIT)
-    return;
-
-  RenderProcessHost* renderer_host =
-      GetRenderFrameHost()->GetRenderWidgetHost()->GetProcess();
-  if (!render_process_blocked_state_changed_subscription_) {
-    render_process_blocked_state_changed_subscription_ =
-        renderer_host->RegisterBlockStateChangedCallback(base::BindRepeating(
-            &NavigationHandleImpl::RenderProcessBlockedStateChanged,
-            base::Unretained(this)));
-  }
-  if (!renderer_host->IsBlocked())
-    commit_timeout_timer_.Start(
-        FROM_HERE, g_commit_timeout,
-        base::BindRepeating(&NavigationHandleImpl::OnCommitTimeout,
-                            weak_factory_.GetWeakPtr()));
-}
-
-void NavigationHandleImpl::OnCommitTimeout() {
-  DCHECK_EQ(NavigationRequest::READY_TO_COMMIT, state());
-  render_process_blocked_state_changed_subscription_.reset();
-  GetRenderFrameHost()->GetRenderWidgetHost()->RendererIsUnresponsive(
-      base::BindRepeating(&NavigationHandleImpl::RestartCommitTimeout,
-                          weak_factory_.GetWeakPtr()));
-}
-
-// static
-void NavigationHandleImpl::SetCommitTimeoutForTesting(
-    const base::TimeDelta& timeout) {
-  if (timeout.is_zero())
-    g_commit_timeout = kDefaultCommitTimeout;
-  else
-    g_commit_timeout = timeout;
-}
-
 }  // namespace content

content/browser/frame_host/navigation_handle_impl.h

@@ -14,12 +14,9 @@
 #include <utility>
 #include <vector>
 
-#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/optional.h"
-#include "base/time/time.h"
-#include "base/timer/timer.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigation_request.h"
 #include "content/browser/frame_host/navigation_throttle_runner.h"
@@ -200,10 +197,6 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
     return navigation_request_->GetDeferringThrottleForTesting();
   }
 
-  // Sets the READY_TO_COMMIT -> DID_COMMIT timeout.  Resets the timeout to the
-  // default value if |timeout| is zero.
-  static void SetCommitTimeoutForTesting(const base::TimeDelta& timeout);
-
  private:
   // TODO(clamy): Transform NavigationHandleImplTest into NavigationRequestTest
   // once NavigationHandleImpl has become a wrapper around NavigationRequest.
@@ -222,17 +215,6 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
     return navigation_request_->handle_state();
   }
 
-  // Called if READY_TO_COMMIT -> COMMIT state transition takes an unusually
-  // long time.
-  void OnCommitTimeout();
-
-  // Called by the RenderProcessHost to handle the case when the process
-  // changed its state of being blocked.
-  void RenderProcessBlockedStateChanged(bool blocked);
-
-  void StopCommitTimeout();
-  void RestartCommitTimeout();
-
   // The NavigationRequest that owns this NavigationHandle.
   NavigationRequest* navigation_request_;
 
@@ -246,14 +228,6 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
   std::vector<std::string> removed_request_headers_;
   net::HttpRequestHeaders modified_request_headers_;
 
-  // Timer for detecting an unexpectedly long time to commit a navigation.
-  base::OneShotTimer commit_timeout_timer_;
-
-  // The subscription to the notification of the changing of the render
-  // process's blocked state.
-  std::unique_ptr<base::CallbackList<void(bool)>::Subscription>
-      render_process_blocked_state_changed_subscription_;
-
   // Allows to override response_headers_ in tests.
   // TODO(clamy): Clean this up once the architecture of unit tests is better.
   scoped_refptr<net::HttpResponseHeaders> response_headers_for_testing_;

content/browser/frame_host/navigation_request.cc

@@ -99,6 +99,16 @@ namespace content {
 
 namespace {
 
+// Default timeout for the READY_TO_COMMIT -> COMMIT transition. Chosen
+// initially based on the Navigation.ReadyToCommitUntilCommit UMA, and then
+// refined based on feedback based on CrashExitCodes.Renderer/RESULT_CODE_HUNG.
+constexpr base::TimeDelta kDefaultCommitTimeout =
+    base::TimeDelta::FromSeconds(30);
+
+// Timeout for the READY_TO_COMMIT -> COMMIT transition.
+// Overrideable via SetCommitTimeoutForTesting.
+base::TimeDelta g_commit_timeout = kDefaultCommitTimeout;
+
 // crbug.com/954271: This feature is a part of an ablation study which makes
 // history navigations slower.
 // TODO(altimin): Clean this up after the study finishes.
@@ -2791,7 +2801,7 @@ void NavigationRequest::DidCommitNavigation(
     handle_state_ = DID_COMMIT;
   }
 
-  navigation_handle_->StopCommitTimeout();
+  StopCommitTimeout();
 
   // Record metrics for the time it took to commit the navigation if it was to
   // another document without error.
@@ -2890,7 +2900,7 @@ void NavigationRequest::ReadyToCommitNavigation(bool is_error) {
 
   handle_state_ = READY_TO_COMMIT;
   ready_to_commit_time_ = base::TimeTicks::Now();
-  navigation_handle_->RestartCommitTimeout();
+  RestartCommitTimeout();
 
   if (appcache_handle_)
     appcache_handle_->SetProcessId(render_frame_host_->GetProcess()->GetID());
@@ -2941,4 +2951,55 @@ void NavigationRequest::RunCompleteCallback(
   // destruction.
 }
 
+void NavigationRequest::RenderProcessBlockedStateChanged(bool blocked) {
+  if (blocked)
+    StopCommitTimeout();
+  else
+    RestartCommitTimeout();
+}
+
+void NavigationRequest::StopCommitTimeout() {
+  commit_timeout_timer_.Stop();
+  render_process_blocked_state_changed_subscription_.reset();
+  render_frame_host()->GetRenderWidgetHost()->RendererIsResponsive();
+}
+
+void NavigationRequest::RestartCommitTimeout() {
+  commit_timeout_timer_.Stop();
+  if (handle_state_ >= DID_COMMIT)
+    return;
+
+  RenderProcessHost* renderer_host =
+      render_frame_host()->GetRenderWidgetHost()->GetProcess();
+  if (!render_process_blocked_state_changed_subscription_) {
+    render_process_blocked_state_changed_subscription_ =
+        renderer_host->RegisterBlockStateChangedCallback(base::BindRepeating(
+            &NavigationRequest::RenderProcessBlockedStateChanged,
+            base::Unretained(this)));
+  }
+  if (!renderer_host->IsBlocked()) {
+    commit_timeout_timer_.Start(
+        FROM_HERE, g_commit_timeout,
+        base::BindRepeating(&NavigationRequest::OnCommitTimeout,
+                            weak_factory_.GetWeakPtr()));
+  }
+}
+
+void NavigationRequest::OnCommitTimeout() {
+  DCHECK_EQ(READY_TO_COMMIT, handle_state_);
+  render_process_blocked_state_changed_subscription_.reset();
+  render_frame_host()->GetRenderWidgetHost()->RendererIsUnresponsive(
+      base::BindRepeating(&NavigationRequest::RestartCommitTimeout,
+                          weak_factory_.GetWeakPtr()));
+}
+
+// static
+void NavigationRequest::SetCommitTimeoutForTesting(
+    const base::TimeDelta& timeout) {
+  if (timeout.is_zero())
+    g_commit_timeout = kDefaultCommitTimeout;
+  else
+    g_commit_timeout = timeout;
+}
+
 }  // namespace content

content/browser/frame_host/navigation_request.h

@@ -7,12 +7,15 @@
 
 #include <memory>
 
+#include "base/callback.h"
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/strings/string_util.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_throttle_runner.h"
@@ -465,6 +468,10 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
 
   int64_t navigation_handle_id() { return navigation_handle_id_; }
 
+  // Sets the READY_TO_COMMIT -> DID_COMMIT timeout. Resets the timeout to the
+  // default value if |timeout| is zero.
+  static void SetCommitTimeoutForTesting(const base::TimeDelta& timeout);
+
  private:
   // TODO(clamy): Transform NavigationHandleImplTest into NavigationRequestTest
   // once NavigationHandleImpl has become a wrapper around NavigationRequest.
@@ -746,6 +753,17 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
   // TODO(zetamoo): This can be removed once the navigation states are merged.
   void RunCompleteCallback(NavigationThrottle::ThrottleCheckResult result);
 
+  // Called if READY_TO_COMMIT -> COMMIT state transition takes an unusually
+  // long time.
+  void OnCommitTimeout();
+
+  // Called by the RenderProcessHost to handle the case when the process changed
+  // its state of being blocked.
+  void RenderProcessBlockedStateChanged(bool blocked);
+
+  void StopCommitTimeout();
+  void RestartCommitTimeout();
+
   FrameTreeNode* frame_tree_node_;
 
   // Invariant: At least one of |loader_| or |render_frame_host_| is null.
@@ -966,6 +984,14 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
   // corresponding provider is created in the renderer.
   std::unique_ptr<ServiceWorkerNavigationHandle> service_worker_handle_;
 
+  // Timer for detecting an unexpectedly long time to commit a navigation.
+  base::OneShotTimer commit_timeout_timer_;
+
+  // The subscription to the notification of the changing of the render
+  // process's blocked state.
+  std::unique_ptr<base::CallbackList<void(bool)>::Subscription>
+      render_process_blocked_state_changed_subscription_;
+
   base::WeakPtrFactory<NavigationRequest> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(NavigationRequest);

content/browser/site_per_process_browsertest.cc

@@ -13922,7 +13922,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
 
   // Attempt to navigate the second tab to a.com.  This will attempt to reuse
   // the hung process.
-  NavigationHandleImpl::SetCommitTimeoutForTesting(
+  NavigationRequest::SetCommitTimeoutForTesting(
       base::TimeDelta::FromMilliseconds(100));
   GURL hung_url(embedded_test_server()->GetURL("a.com", "/title3.html"));
   UnresponsiveRendererObserver unresponsive_renderer_observer(new_contents);
@@ -13935,7 +13935,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   EXPECT_EQ(hung_process, a_process);
 
   // Reset the timeout.
-  NavigationHandleImpl::SetCommitTimeoutForTesting(base::TimeDelta());
+  NavigationRequest::SetCommitTimeoutForTesting(base::TimeDelta());
 }
 
 // This is a regression test for https://crbug.com/881812 which complained that
@@ -13973,7 +13973,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   // Attempt to navigate the second tab to a.com.  This will attempt to reuse
   // the hung process.
   base::TimeDelta kTimeout = base::TimeDelta::FromMilliseconds(100);
-  NavigationHandleImpl::SetCommitTimeoutForTesting(kTimeout);
+  NavigationRequest::SetCommitTimeoutForTesting(kTimeout);
   GURL hung_url(embedded_test_server()->GetURL("a.com", "/title3.html"));
   UnresponsiveRendererObserver unresponsive_renderer_observer(new_contents);
   EXPECT_TRUE(
@@ -13987,7 +13987,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   EXPECT_FALSE(hung_process);
 
   // Reset the timeout.
-  NavigationHandleImpl::SetCommitTimeoutForTesting(base::TimeDelta());
+  NavigationRequest::SetCommitTimeoutForTesting(base::TimeDelta());
 }
 
 // Tests that an inner WebContents will reattach to its outer WebContents after

content/browser/site_per_process_unload_browsertest.cc

@@ -349,7 +349,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   // Attempt to navigate the second tab to a.com.  This will attempt to reuse
   // the hung process.
   base::TimeDelta kTimeout = base::TimeDelta::FromMilliseconds(100);
-  NavigationHandleImpl::SetCommitTimeoutForTesting(kTimeout);
+  NavigationRequest::SetCommitTimeoutForTesting(kTimeout);
   GURL hung_url(embedded_test_server()->GetURL("a.com", "/title3.html"));
   UnresponsiveRendererObserver unresponsive_renderer_observer(new_contents);
   EXPECT_TRUE(
@@ -363,7 +363,7 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   EXPECT_FALSE(hung_process);
 
   // Reset the timeout.
-  NavigationHandleImpl::SetCommitTimeoutForTesting(base::TimeDelta());
+  NavigationRequest::SetCommitTimeoutForTesting(base::TimeDelta());
 }
 
 // Test that unload handlers in iframes are run, even when the removed subtree