NavigationSimulator: Refactor of WaitForThrottleChecksComplete

The end goal is to get NavigationSimulator to a state where we can call:
simulator->SetAutoAdvance(false);
simulator->Start();

And have the simulator *not* spin the run loop in the background. This
will help in writing more sophisticated NavigationThrottle tests which
use multiple task runners to execute async tasks.

This particular CL does a few things:
1. Removes base::RunLoop().RunUntilIdle call. This is not great practice
   to have in test infra, since it can cause strange interactions with
   tests and they may start relying on unintentional behavior.

2. Collects various after-throttle checks into closures, to be called
   when the throttle check complete closure is called (rather than
   relying on base::RunLoop to keep everything on the stack). This
   isn't so useful now, but it will if we stop *always* waiting.

3. Slightly modifies checks in various places that relied on some of
   the behavior that we've changed. Two things changed here:
   a. "Complete" code internal to the NavigationThrottle now runs
      directly after throttle checks are complete (when
      complete_callback_for_testing_ is called on the nav handle),
      rather than when the RunLoop quits. This caused us to remove
      one check checking that we've finished the navigation.

   b. Because we no longer RunUntilIdle() in
      WaitForThrottleChecksComplete, code that relies on the nav finishing
      after the post-task in NavigationRequest::OnStartChecksComplete
      will need to either wait or spin the run loop to see the end of the
      navigation. This is a pretty small edge case and it is pretty
      easy to either write a helper to make this explicit or use a WCO.
      The only test impacted by this change is a NavigationSimulator test,
      and a test which incidentally relied on this behavior for unrelated
      reasons.

Bug: 822275

Change-Id: Ice6b215fa285a6a944fdf85108303515d109be77
Reviewed-on: https://chromium-review.googlesource.com/961819Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546184}

chrome/browser/resource_coordinator/tab_activity_watcher_unittest.cc

@@ -308,6 +308,10 @@ TEST_F(TabMetricsTest, InputEvents) {
   content::WebContents* test_contents_2 =
       tab_activity_simulator_.AddWebContentsAndNavigate(tab_strip_model,
                                                         GURL(kTestUrls[1]));
+
+  // RunUntilIdle is needed because the widget input handler is initialized
+  // asynchronously via mojo (see SetupWidgetInputHandler).
+  base::RunLoop().RunUntilIdle();
   tab_strip_model->ActivateTabAt(0, false);
 
   UkmMetricMap expected_metrics_1(kBasicMetricValues);

components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc

@@ -505,10 +505,12 @@ TEST_P(ActivationStateComputingThrottleSubFrameTest, Speculation) {
   base::HistogramTester main_histogram_tester;
   CreateTestNavigationForMainFrame(GURL("http://example.test/"));
   SimulateStartAndExpectToProceed();
+  base::RunLoop().RunUntilIdle();
   int main_frame_checks = dryrun_speculation() ? 1 : 0;
   main_histogram_tester.ExpectTotalCount(kActivationCPU, main_frame_checks);
 
   SimulateRedirectAndExpectToProceed(GURL("http://example.test2/"));
+  base::RunLoop().RunUntilIdle();
   main_frame_checks += dryrun_speculation() ? 1 : 0;
   main_histogram_tester.ExpectTotalCount(kActivationCPU, main_frame_checks);
 
@@ -523,9 +525,11 @@ TEST_P(ActivationStateComputingThrottleSubFrameTest, Speculation) {
                                       last_activation_state());
   // For subframes, do a ruleset lookup at the start and every redirect.
   SimulateStartAndExpectToProceed();
+  base::RunLoop().RunUntilIdle();
   sub_histogram_tester.ExpectTotalCount(kActivationCPU, 1);
 
   SimulateRedirectAndExpectToProceed(GURL("http://example.test2/"));
+  base::RunLoop().RunUntilIdle();
   sub_histogram_tester.ExpectTotalCount(kActivationCPU, 2);
 
   // No ruleset lookup required at commit because we've already checked the

content/public/test/navigation_simulator.cc

@@ -4,6 +4,8 @@
 
 #include "content/public/test/navigation_simulator.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
@@ -282,8 +284,11 @@ void NavigationSimulator::Start() {
     return;
   }
 
-  WaitForThrottleChecksComplete();
+  WaitForThrottleChecksComplete(base::BindOnce(
+      &NavigationSimulator::StartComplete, weak_factory_.GetWeakPtr()));
+}
 
+void NavigationSimulator::StartComplete() {
   CHECK_EQ(1, num_did_start_navigation_called_);
   if (GetLastThrottleCheckResult().action() == NavigationThrottle::PROCEED) {
     CHECK_EQ(1, num_will_start_request_called_);
@@ -332,8 +337,15 @@ void NavigationSimulator::Redirect(const GURL& new_url) {
       redirect_info,
       scoped_refptr<network::ResourceResponse>(new network::ResourceResponse));
 
-  WaitForThrottleChecksComplete();
+  WaitForThrottleChecksComplete(base::BindOnce(
+      &NavigationSimulator::RedirectComplete, weak_factory_.GetWeakPtr(),
+      previous_num_will_redirect_request_called,
+      previous_did_redirect_navigation_called));
+}
 
+void NavigationSimulator::RedirectComplete(
+    int previous_num_will_redirect_request_called,
+    int previous_did_redirect_navigation_called) {
   if (GetLastThrottleCheckResult().action() == NavigationThrottle::PROCEED) {
     CHECK_EQ(previous_num_will_redirect_request_called + 1,
              num_will_redirect_request_called_);
@@ -370,10 +382,21 @@ void NavigationSimulator::ReadyToCommit() {
     return;
   }
 
-  if (!same_document_ && !navigation_url_.IsAboutBlank() &&
-      IsURLHandledByNetworkStack(navigation_url_)) {
-    WaitForThrottleChecksComplete();
+  bool needs_throttle_checks = !same_document_ &&
+                               !navigation_url_.IsAboutBlank() &&
+                               IsURLHandledByNetworkStack(navigation_url_);
+  auto complete_closure =
+      base::BindOnce(&NavigationSimulator::ReadyToCommitComplete,
+                     weak_factory_.GetWeakPtr(), needs_throttle_checks);
+  if (needs_throttle_checks) {
+    WaitForThrottleChecksComplete(std::move(complete_closure));
+    return;
+  }
+  std::move(complete_closure).Run();
+}
 
+void NavigationSimulator::ReadyToCommitComplete(bool ran_throttles) {
+  if (ran_throttles) {
     if (GetLastThrottleCheckResult().action() != NavigationThrottle::PROCEED) {
       state_ = FAILED;
       return;
@@ -382,7 +405,6 @@ void NavigationSimulator::ReadyToCommit() {
     CHECK_EQ(1, num_ready_to_commit_called_);
   }
 
-
   request_id_ = handle_->GetGlobalRequestID();
 
   // Update the RenderFrameHost now that we know which RenderFrameHost will
@@ -469,7 +491,6 @@ void NavigationSimulator::Fail(int error_code) {
 
   state_ = FAILED;
 
-  bool should_result_in_error_page = error_code != net::ERR_ABORTED;
   PrepareCompleteCallbackOnHandle();
   NavigationRequest* request = frame_tree_node_->navigation_request();
   CHECK(request);
@@ -477,8 +498,20 @@ void NavigationSimulator::Fail(int error_code) {
       static_cast<TestNavigationURLLoader*>(request->loader_for_testing());
   CHECK(url_loader);
   url_loader->SimulateError(error_code);
+
+  auto complete_closure =
+      base::BindOnce(&NavigationSimulator::FailComplete,
+                     weak_factory_.GetWeakPtr(), error_code);
+  if (error_code != net::ERR_ABORTED) {
+    WaitForThrottleChecksComplete(std::move(complete_closure));
+    return;
+  }
+  std::move(complete_closure).Run();
+}
+
+void NavigationSimulator::FailComplete(int error_code) {
+  bool should_result_in_error_page = error_code != net::ERR_ABORTED;
   if (error_code != net::ERR_ABORTED) {
-    WaitForThrottleChecksComplete();
     NavigationThrottle::ThrottleCheckResult result =
         GetLastThrottleCheckResult();
     if (result.action() == NavigationThrottle::CANCEL ||
@@ -494,8 +527,6 @@ void NavigationSimulator::Fail(int error_code) {
     // commit the error page.
     render_frame_host_ =
         static_cast<TestRenderFrameHost*>(handle_->GetRenderFrameHost());
-  } else {
-    CHECK_EQ(1, num_did_finish_navigation_called_);
   }
 }
 
@@ -838,27 +869,29 @@ bool NavigationSimulator::SimulateRendererInitiatedStart() {
   return true;
 }
 
-void NavigationSimulator::WaitForThrottleChecksComplete() {
+void NavigationSimulator::WaitForThrottleChecksComplete(
+    base::OnceClosure complete_closure) {
   // If last_throttle_check_result_ is set, then throttle checks completed
   // synchronously.
-  if (!last_throttle_check_result_) {
-    base::RunLoop run_loop;
-    throttle_checks_wait_closure_ = run_loop.QuitClosure();
-    run_loop.Run();
-    throttle_checks_wait_closure_.Reset();
+  if (last_throttle_check_result_) {
+    std::move(complete_closure).Run();
+    return;
   }
 
-  // Run message loop once since NavigationRequest::OnStartChecksComplete posted
-  // a task.
-  base::RunLoop().RunUntilIdle();
+  throttle_checks_complete_closure_ = std::move(complete_closure);
+  base::RunLoop run_loop;
+  wait_closure_ = run_loop.QuitClosure();
+  run_loop.Run();
 }
 
 void NavigationSimulator::OnThrottleChecksComplete(
     NavigationThrottle::ThrottleCheckResult result) {
   DCHECK(!last_throttle_check_result_);
   last_throttle_check_result_ = result;
-  if (throttle_checks_wait_closure_)
-    throttle_checks_wait_closure_.Run();
+  if (wait_closure_)
+    std::move(wait_closure_).Run();
+  if (throttle_checks_complete_closure_)
+    std::move(throttle_checks_complete_closure_).Run();
 }
 
 void NavigationSimulator::PrepareCompleteCallbackOnHandle() {

content/public/test/navigation_simulator.h

@@ -269,6 +269,12 @@ class NavigationSimulator : public WebContentsObserver {
   void ReadyToCommitNavigation(NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(NavigationHandle* navigation_handle) override;
 
+  void StartComplete();
+  void RedirectComplete(int previous_num_will_redirect_request_called,
+                        int previous_did_redirect_navigation_called);
+  void ReadyToCommitComplete(bool ran_throttles);
+  void FailComplete(int error_code);
+
   void OnWillStartRequest();
   void OnWillRedirectRequest();
   void OnWillFailRequest();
@@ -283,10 +289,11 @@ class NavigationSimulator : public WebContentsObserver {
   bool SimulateRendererInitiatedStart();
 
   // This method will block waiting for throttle checks to complete.
-  void WaitForThrottleChecksComplete();
+  void WaitForThrottleChecksComplete(base::OnceClosure complete_closure);
 
-  // Sets |last_throttle_check_result_| and calls
-  // |throttle_checks_wait_closure_|.
+  // Sets |last_throttle_check_result_| and calls both the
+  // |wait_closure_| and the |throttle_checks_complete_closure_|, if they are
+  // set.
   void OnThrottleChecksComplete(NavigationThrottle::ThrottleCheckResult result);
 
   // Helper method to set the OnThrottleChecksComplete callback on the
@@ -361,8 +368,13 @@ class NavigationSimulator : public WebContentsObserver {
   // WillProcessResponse has been invoked on the NavigationHandle.
   content::GlobalRequestID request_id_;
 
-  // Closure that is set when WaitForThrottleChecksComplete is called.
-  base::Closure throttle_checks_wait_closure_;
+  // Closure that is set when WaitForThrottleChecksComplete is called. Called in
+  // OnThrottleChecksComplete.
+  base::OnceClosure throttle_checks_complete_closure_;
+
+  // Closure that is called in OnThrottleChecksComplete if we are waiting on the
+  // result. Calling this will quit the nested run loop.
+  base::OnceClosure wait_closure_;
 
   base::WeakPtrFactory<NavigationSimulator> weak_factory_;
 };

content/test/navigation_simulator_unittest.cc

@@ -46,7 +46,7 @@ class NavigationSimulatorTest
   }
 
   void TearDown() override {
-    EXPECT_EQ(expect_navigation_to_finish_, did_finish_navigation_);
+    EXPECT_TRUE(did_finish_navigation_);
     RenderViewHostImplTestHarness::TearDown();
   }
 
@@ -73,7 +73,6 @@ class NavigationSimulatorTest
   base::Optional<TestNavigationThrottle::ThrottleMethod> cancel_time_;
   TestNavigationThrottle::ResultSynchrony sync_;
   std::unique_ptr<NavigationSimulator> simulator_;
-  bool expect_navigation_to_finish_ = true;
   bool did_finish_navigation_ = false;
   bool will_fail_request_called_ = false;
   base::WeakPtrFactory<NavigationSimulatorTest> weak_ptr_factory_;
@@ -95,6 +94,10 @@ TEST_P(NavigationSimulatorTest, Cancel) {
       cancel_time_.value() == TestNavigationThrottle::WILL_START_REQUEST) {
     EXPECT_EQ(NavigationThrottle::CANCEL,
               simulator_->GetLastThrottleCheckResult());
+    // NavigationRequest::OnStartChecksComplete will post a task to finish the
+    // navigation, so pump the run loop here to ensure checks in TearDown()
+    // succeed.
+    base::RunLoop().RunUntilIdle();
     return;
   }
   EXPECT_EQ(NavigationThrottle::PROCEED,