Allow asynchronous deferral in NavigationSimulator

content/public/test/navigation_simulator.h

Index: content/public/test/navigation_simulator.h
diff --git a/content/public/test/navigation_simulator.h b/content/public/test/navigation_simulator.h
index 8c37ab7a34c3b1f147a1bec853128ee82a81a79c..1a6ba01dc470f6aa2a2ca5b93b549cf9f1c32b3f 100644
--- a/content/public/test/navigation_simulator.h
+++ b/content/public/test/navigation_simulator.h
@@ -7,6 +7,9 @@
 
 #include <memory>
 
+#include "base/callback.h"
+#include "base/optional.h"
+#include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/referrer.h"
 #include "content/public/test/navigation_simulator.h"
@@ -21,6 +24,10 @@ class RenderFrameHost;
 class TestRenderFrameHost;
 struct Referrer;
 
+// To avoid DEPS violation by bringing in navigation_handle_impl.h.
+using ThrottleChecksFinishedCallback =
+    base::Callback<void(NavigationThrottle::ThrottleCheckResult)>;
+
 // An interface for simulating a navigation in unit tests. Currently this only
 // supports renderer-initiated navigations.
 // Note: this should not be used in browser tests.
@@ -64,19 +71,25 @@ class NavigationSimulator : public WebContentsObserver {
   //       NavigationSimulator::CreateRendererInitiated(
   //           original_url, render_frame_host);
   //   simulator->SetTransition(ui::PAGE_TRANSITION_LINK);
-  //   simulator->Start();
-  //   for (GURL redirect_url : redirects)
-  //     simulator->Redirect(redirect_url);
-  //   simulator->Commit();
+  //   if (simulator->Start() == NavigationThrottle::DEFER)

[clamy, 2017/03/06 15:22:15]

In fact, that's not the interface I had in mind. What I wanted was for Start,
Redirect and Commit to block until all asynchronous checks have been resolved.
This way callers need not to worry about whether checks are asynchronous or not,
and just get notified of what the end result was.

So basically, that means folding WaitForThrottleChecksComplete in all of these
methods, so that we always wait for the throttle checks to complete. Does this
make sense?

[Charlie Harrison, 2017/03/06 20:18:43]

As discussed offline, this isn't possible with how we've designed subresource
filter unit tests. These tests post a series of messages on *another* task
runner which must complete before the navigation is resumed. So our code
basically does:

Start();
blocking_task_runner_->RunUntilIdle();
WaitForThrottleChecksComplete();

And would hang if we omit the RunUntilIdle call.

[engedy, 2017/03/07 15:24:56]

I found it practical to look at this from the following angle: what would be the
best interface for the NavigationSimulator so that it can be nicely used to test
two throttles at the same time (with both throttles doing async work in the same
stage)?

While the current approach indeed allows verifying that things take place on the
correct threads, it is a bit awkward:
It looks like that for a NavigationThrottles wishing to do async processing
usually do it on:
 1.) a TaskRunner specified at construction time (which can be set up in unit
tests so that they are aliases of a single MessageLoop),
 2.) Named content::BrowserThreads, which can be easily made real threads if
there is no other solution,
 3.) Blocking pools (not sure how these work with TestBrowserThreadBundle, but
in the worst case, we could expose a method like [1] and let tests subclass
NavigationSimulator, although I am not sure that's too nice either).

The threading constraints could be checked at a different layer (or some other
way, or not at all?), and then we would not need two flavors of these methods
either.

[1]:
https://cs.chromium.org/chromium/src/base/test/test_mock_time_task_runner.h?r...

[Charlie Harrison, 2017/03/07 16:42:43]

I see what you are saying. I am in favor of this approach (1) in this CL. If
consumers need (2) or (3) and don't like the proposed solutions we can think of
making NavigationSimulator more complicated.

[engedy, 2017/03/09 11:41:34]

Yep, you understood correctly, but I should have phrased this better.

To clarify, I did not mean to suggest using approaches (2) or (3) in this CL, I
merely wanted to point out that in general, clients wishing to use
NavigationSimulator will probably have some async behavior that falls into one
of these categories (i.e. 1, 2, 3), so they will likely still be able to make
good use of the simpler NavigationSimulator interface.

My other pet peeve with the old solution was that a construct like this:

Start();
blocking_task_runner_->RunUntilIdle();
WaitForThrottleChecksComplete();

does not actually verify that the async tasks are posted to
|blocking_task_runner_| and not to the main MessageLoop. Furthermore, I think
that because of crbug.com/631186, even if the classes being exercised (e.g.
VerifiedRuleset) use a ThreadChecker inside, not even that will catch a
threading problem in unittests.

[Charlie Harrison, 2017/03/09 14:36:01]

Thanks for the clarification.

+  //     simulator->WaitForThrottleChecksComplete();
+  //   for (GURL redirect_url : redirects) {
+  //     EXPECT_EQ(NavigationThrottle::PROCEED,
+  //               simulator->Redirect(redirect_url));
+  //   }
+  //   EXPECT_EQ(NavigationThrottle::PROCEED, simulator->Commit());
   //
   // Example of usage for a failed renderer-initiated navigation:
   //   unique_ptr<NavigationSimulator> simulator =
   //       NavigationSimulator::CreateRendererInitiated(
   //           original_url, render_frame_host);
   //   simulator->SetTransition(ui::PAGE_TRANSITION_LINK);
-  //   simulator->Start();
-  //   for (GURL redirect_url : redirects)
-  //     simulator->Redirect(redirect_url);
+  //   EXPECT_EQ(NavigationThrottle::PROCEED, simulator->Start());
+  //   for (GURL redirect_url : redirects) {
+  //     EXPECT_EQ(NavigationThrottle::DEFER,
+  //               simulator->Redirect(redirect_url));
+  //     simulator->WaitForThrottleChecksComplete();
+  //   }
   //   simulator->Fail(net::ERR_TIMED_OUT);
   //   simulator->CommitErrorPage();
   //
@@ -87,13 +100,13 @@ class NavigationSimulator : public WebContentsObserver {
   //   simulator->CommitSamePage();
 
   // Simulates the start of the navigation.
-  virtual void Start();
+  virtual NavigationThrottle::ThrottleCheckResult Start();
 
   // Simulates a redirect to |new_url| for the navigation.
-  virtual void Redirect(const GURL& new_url);
+  virtual NavigationThrottle::ThrottleCheckResult Redirect(const GURL& new_url);
 
   // Simulates the commit of the navigation in the RenderFrameHost.
-  virtual void Commit();
+  virtual NavigationThrottle::ThrottleCheckResult Commit();
 
   // Simulates the navigation failing with the error code |error_code|.
   virtual void Fail(int error_code);
@@ -122,6 +135,11 @@ class NavigationSimulator : public WebContentsObserver {
   // |Redirect|.
   virtual void SetReferrer(const Referrer& referrer);
 
+  // This method will wait for throttle checks to complete if the simulator is
+  // running in AllowThrottleChecks mode.

[clamy, 2017/03/06 15:22:15]

We no longer have a non AllowThrottleChecks mode :).

[Charlie Harrison, 2017/03/06 20:18:43]

Done.

+  virtual NavigationThrottle::ThrottleCheckResult
+  WaitForThrottleChecksComplete();
+
  private:
   // WebContentsObserver:
   void DidStartNavigation(NavigationHandle* navigation_handle) override;
@@ -133,6 +151,27 @@ class NavigationSimulator : public WebContentsObserver {
   void OnWillRedirectRequest();
   void OnWillProcessResponse();
 
+  // Called after all the throttle checks are finished for these steps.
+  void FinishStart(NavigationThrottle::ThrottleCheckResult result);
+  void FinishRedirect(int previous_num_will_redirect_request_called,
+                      int previous_did_redirect_navigation_called,
+                      NavigationThrottle::ThrottleCheckResult result);
+  void FinishCommit(NavigationThrottle::ThrottleCheckResult result);
+
+  // Returns the sync result of the throttle checks if it is available, or DEFER
+  // if it is not.
+  NavigationThrottle::ThrottleCheckResult RunOrStoreThrottleCompleteCallback(
+      ThrottleChecksFinishedCallback finish_callback);
+
+  void OnThrottleChecksComplete(NavigationThrottle::ThrottleCheckResult result);
+
+  // Helper method to get the OnThrottleChecksComplete callback.
+  ThrottleChecksFinishedCallback GenerateCompleteCallback();
+
+  // Helper method for places in the code which expect the navigation to start
+  // successfuly (e.g. when calling Commit() before Start()).
+  void StartAndExpectToProceed();
+
   enum State {
     INITIALIZATION,
     STARTED,
@@ -162,6 +201,18 @@ class NavigationSimulator : public WebContentsObserver {
   int num_ready_to_commit_called_ = 0;
   int num_did_finish_navigation_called_ = 0;
 
+  // Holds the last ThrottleCheckResult calculated by the navigation's
+  // throttles. Will be unset before WillStartRequest is finished. Will be unset
+  // while throttles are being run, but before they finish.
+  base::Optional<NavigationThrottle::ThrottleCheckResult>
+      last_throttle_check_result_;
+
+  // Used to execute the Finish* methods, after the throttles have all finished.
+  ThrottleChecksFinishedCallback complete_callback_;
+
+  // Closure that is set when WaitForThrottleChecksComplete is called.
+  base::Closure throttle_checks_wait_closure_;
+
   base::WeakPtrFactory<NavigationSimulator> weak_factory_;
 };