Allow asynchronous deferral in NavigationSimulator

content/public/test/navigation_simulator.h

 // Copyright 2017 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.
 
 #ifndef CONTENT_PUBLIC_TEST_NAVIGATION_SIMULATOR_H_
 #define CONTENT_PUBLIC_TEST_NAVIGATION_SIMULATOR_H_
 
 #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"
 #include "ui/base/page_transition_types.h"
 
 class GURL;
 
 namespace content {
 
 class NavigationHandleImpl;
 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.
 // TODO(clamy): support browser-initiated navigations.
 class NavigationSimulator : public WebContentsObserver {
  public:
   // Simulates a renderer-initiated navigation to |url| started in
   // |render_frame_host| from start to commit.
   static void NavigateAndCommitFromDocument(const GURL& original_url,
                                             RenderFrameHost* render_frame_host);
@@ skipping 23 matching lines @@
   // --------------------------------------------------------------------------
 
   // The following functions should be used to simulate events happening during
   // a navigation.
   //
   // Example of usage for a successful 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);
-  //   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();
   //
   // Example of usage for a same-page renderer-initiated navigation:
   //   unique_ptr<NavigationSimulator> simulator =
   //       NavigationSimulator::CreateRendererInitiated(
   //           original_url, render_frame_host);
   //   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);
 
   // Simulates the commit of an error page following a navigation failure.
   virtual void CommitErrorPage();
 
   // Simulates the commit of a same-page navigation, ie fragment navigations or
   // pushState/popState navigations.
   virtual void CommitSamePage();
 
   // --------------------------------------------------------------------------
 
   // The following functions are used to specify the parameters of the
   // navigation. Changes should be  made before calling |Start|, unless they are
   // meant to apply to a redirect. In that case, they should be made before
   // calling |Redirect|.
 
   // The following parameters are constant during the navigation and may only be
   // specified before calling |Start|.
   virtual void SetTransition(ui::PageTransition transition);
 
   // The following parameters can change during redirects. They should be
   // specified before calling |Start| if they need to apply to the navigation to
   // the original url. Otherwise, they should be specified before calling
   // |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;
   void DidRedirectNavigation(NavigationHandle* navigation_handle) override;
   void ReadyToCommitNavigation(NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(NavigationHandle* navigation_handle) override;
 
   void OnWillStartRequest();
   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,
     FAILED,
     FINISHED,
   };
 
   State state_ = INITIALIZATION;
 
   // The renderer associated with this navigation.
   TestRenderFrameHost* render_frame_host_;
 
   // The NavigationHandle associated with this navigation.
   NavigationHandleImpl* handle_;
 
   GURL navigation_url_;
   Referrer referrer_;
   ui::PageTransition transition_ = ui::PAGE_TRANSITION_LINK;
 
   // These are used to sanity check the content/public/ API calls emitted as
   // part of the navigation.
   int num_did_start_navigation_called_ = 0;
   int num_will_start_request_called_ = 0;
   int num_will_redirect_request_called_ = 0;
   int num_did_redirect_navigation_called_ = 0;
   int num_will_process_response_called_ = 0;
   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_;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_TEST_NAVIGATION_SIMULATOR_H_