Make the intercept navigation throttle async

Design doc: https://bit.ly/2JgkZyc

This CL changes the intercept navigation throttle to avoid blocking
network requests from starting. Instead, policy decisions are computed
asynchronously, and the load is canceled+ignored at the next
NavigationThrottle callback.

Note that in cases where the request should be ignored, this wastes
some bytes. However, this is a pretty rare case. It looks like ~99% of
the time we won't cancel the navigation (from
Android.TabNavigationInterceptResult)

Bug: 793053
Change-Id: I9bf4eb29cf7ff92d31432eedb41cd446f738a492
Reviewed-on: https://chromium-review.googlesource.com/815354
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565764}

components/navigation_interception/BUILD.gn

@@ -51,6 +51,7 @@ source_set("unit_tests") {
   deps = [
     ":navigation_interception",
     "//base",
+    "//base/test:test_support",
     "//content/public/browser",
     "//content/test:test_support",
     "//testing/gmock",

components/navigation_interception/intercept_navigation_throttle.cc

@@ -4,31 +4,38 @@
 
 #include "components/navigation_interception/intercept_navigation_throttle.h"
 
+#include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/timer/elapsed_timer.h"
-#include "components/navigation_interception/navigation_params.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
-
-using content::BrowserThread;
+#include "url/gurl.h"
 
 namespace navigation_interception {
 
+const base::Feature InterceptNavigationThrottle::kAsyncCheck{
+    "AsyncNavigationIntercept", base::FEATURE_DISABLED_BY_DEFAULT};
+
 InterceptNavigationThrottle::InterceptNavigationThrottle(
     content::NavigationHandle* navigation_handle,
     CheckCallback should_ignore_callback)
     : content::NavigationThrottle(navigation_handle),
-      should_ignore_callback_(should_ignore_callback) {}
+      should_ignore_callback_(should_ignore_callback),
+      ui_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_factory_(this) {}
 
-InterceptNavigationThrottle::~InterceptNavigationThrottle() {}
+InterceptNavigationThrottle::~InterceptNavigationThrottle() {
+  UMA_HISTOGRAM_BOOLEAN("Navigation.Intercept.Ignored", should_ignore_);
+}
 
 content::NavigationThrottle::ThrottleCheckResult
 InterceptNavigationThrottle::WillStartRequest() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(!should_ignore_);
   base::ElapsedTimer timer;
 
-  auto result = CheckIfShouldIgnoreNavigation(false);
+  auto result = CheckIfShouldIgnoreNavigation(false /* is_redirect */);
   UMA_HISTOGRAM_COUNTS_10M("Navigation.Intercept.WillStart",
                            timer.Elapsed().InMicroseconds());
   return result;
@@ -36,28 +43,89 @@ InterceptNavigationThrottle::WillStartRequest() {
 
 content::NavigationThrottle::ThrottleCheckResult
 InterceptNavigationThrottle::WillRedirectRequest() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  return CheckIfShouldIgnoreNavigation(true);
+  if (should_ignore_)
+    return content::NavigationThrottle::CANCEL_AND_IGNORE;
+  return CheckIfShouldIgnoreNavigation(true /* is_redirect */);
+}
+
+content::NavigationThrottle::ThrottleCheckResult
+InterceptNavigationThrottle::WillFailRequest() {
+  return WillFinish();
+}
+
+content::NavigationThrottle::ThrottleCheckResult
+InterceptNavigationThrottle::WillProcessResponse() {
+  return WillFinish();
 }
 
 const char* InterceptNavigationThrottle::GetNameForLogging() {
   return "InterceptNavigationThrottle";
 }
 
+content::NavigationThrottle::ThrottleCheckResult
+InterceptNavigationThrottle::WillFinish() {
+  DCHECK(!deferring_);
+  if (should_ignore_)
+    return content::NavigationThrottle::CANCEL_AND_IGNORE;
+
+  if (pending_checks_ > 0) {
+    deferring_ = true;
+    return content::NavigationThrottle::DEFER;
+  }
+
+  return content::NavigationThrottle::PROCEED;
+}
+
 content::NavigationThrottle::ThrottleCheckResult
 InterceptNavigationThrottle::CheckIfShouldIgnoreNavigation(bool is_redirect) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  NavigationParams navigation_params(
+  pending_checks_++;
+  if (ShouldCheckAsynchronously()) {
+    ui_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&InterceptNavigationThrottle::RunCheck,
+                                  weak_factory_.GetWeakPtr(),
+                                  GetNavigationParams(is_redirect)));
+    return content::NavigationThrottle::PROCEED;
+  }
+  RunCheck(GetNavigationParams(is_redirect));
+  return should_ignore_ ? content::NavigationThrottle::CANCEL_AND_IGNORE
+                        : content::NavigationThrottle::PROCEED;
+}
+
+void InterceptNavigationThrottle::RunCheck(const NavigationParams& params) {
+  should_ignore_ |= should_ignore_callback_.Run(
+      navigation_handle()->GetWebContents(), params);
+  DCHECK_GT(pending_checks_, 0);
+  pending_checks_--;
+  if (!deferring_ || pending_checks_ > 0)
+    return;
+
+  if (should_ignore_) {
+    CancelDeferredNavigation(content::NavigationThrottle::CANCEL_AND_IGNORE);
+  } else {
+    Resume();
+  }
+}
+
+bool InterceptNavigationThrottle::ShouldCheckAsynchronously() const {
+  // Do not apply the async optimization for:
+  // - POST navigations, to ensure we aren't violating idempotency.
+  // - Subframe navigations, which aren't observed on Android, and should be
+  //   fast on other platforms.
+  // - non-http/s URLs, which are more likely to be intercepted.
+  return navigation_handle()->IsInMainFrame() &&
+         !navigation_handle()->IsPost() &&
+         navigation_handle()->GetURL().SchemeIsHTTPOrHTTPS() &&
+         base::FeatureList::IsEnabled(kAsyncCheck);
+}
+
+NavigationParams InterceptNavigationThrottle::GetNavigationParams(
+    bool is_redirect) const {
+  return NavigationParams(
       navigation_handle()->GetURL(), navigation_handle()->GetReferrer(),
       navigation_handle()->HasUserGesture(), navigation_handle()->IsPost(),
       navigation_handle()->GetPageTransition(), is_redirect,
       navigation_handle()->IsExternalProtocol(), true,
       navigation_handle()->GetBaseURLForDataURL());
-  bool should_ignore_navigation = should_ignore_callback_.Run(
-      navigation_handle()->GetWebContents(), navigation_params);
-  return should_ignore_navigation
-             ? content::NavigationThrottle::CANCEL_AND_IGNORE
-             : content::NavigationThrottle::PROCEED;
 }
 
 }  // namespace navigation_interception

components/navigation_interception/intercept_navigation_throttle.h

@@ -5,11 +5,13 @@
 #ifndef COMPONENTS_NAVIGATION_INTERCEPTION_INTERCEPT_NAVIGATION_THROTTLE_H_
 #define COMPONENTS_NAVIGATION_INTERCEPTION_INTERCEPT_NAVIGATION_THROTTLE_H_
 
-#include <string>
-
 #include "base/callback.h"
+#include "base/feature_list.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
+#include "components/navigation_interception/navigation_params.h"
 #include "content/public/browser/navigation_throttle.h"
 
 namespace content {
@@ -25,10 +27,13 @@ class NavigationParams;
 // level navigations. This is a UI thread class.
 class InterceptNavigationThrottle : public content::NavigationThrottle {
  public:
-  typedef base::Callback<bool(content::WebContents* /* source */,
-                              const NavigationParams& /* navigation_params */)>
+  typedef base::RepeatingCallback<bool(
+      content::WebContents* /* source */,
+      const NavigationParams& /* navigation_params */)>
       CheckCallback;
 
+  static const base::Feature kAsyncCheck;
+
   InterceptNavigationThrottle(content::NavigationHandle* navigation_handle,
                               CheckCallback should_ignore_callback);
   ~InterceptNavigationThrottle() override;
@@ -36,13 +41,43 @@ class InterceptNavigationThrottle : public content::NavigationThrottle {
   // content::NavigationThrottle implementation:
   ThrottleCheckResult WillStartRequest() override;
   ThrottleCheckResult WillRedirectRequest() override;
+  ThrottleCheckResult WillFailRequest() override;
+  ThrottleCheckResult WillProcessResponse() override;
   const char* GetNameForLogging() override;
 
  private:
+  // To be called on either WillFailRequest or WillProcessResponse.
+  ThrottleCheckResult WillFinish();
+
   ThrottleCheckResult CheckIfShouldIgnoreNavigation(bool is_redirect);
+  void RunCheck(const NavigationParams& params);
+
+  bool ShouldCheckAsynchronously() const;
+
+  // Constructs NavigationParams for this navigation.
+  NavigationParams GetNavigationParams(bool is_redirect) const;
 
+  // This callback should be called at the start of navigation and every
+  // redirect, until |should_ignore_| is true.
   CheckCallback should_ignore_callback_;
 
+  // Note that the CheckCallback currently has thread affinity on the Java side.
+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
+
+  // How many outbound pending checks are running. Normally this will be either
+  // 0 or 1, but making this a bool makes too many assumptions about the nature
+  // of Chrome's task queues (e.g. we could be scheduled after the task which
+  // redirects the navigation).
+  int pending_checks_ = 0;
+
+  // Whether the navigation should be ignored. Updated at every redirect.
+  bool should_ignore_ = false;
+
+  // Whether the navigation is currently deferred.
+  bool deferring_ = false;
+
+  base::WeakPtrFactory<InterceptNavigationThrottle> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(InterceptNavigationThrottle);
 };
 

components/navigation_interception/intercept_navigation_throttle_unittest.cc

@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/test/scoped_feature_list.h"
 #include "components/navigation_interception/navigation_params.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/navigation_throttle.h"
@@ -57,10 +58,19 @@ class MockInterceptCallbackReceiver {
 // InterceptNavigationThrottleTest ------------------------------------
 
 class InterceptNavigationThrottleTest
-    : public content::RenderViewHostTestHarness {
+    : public content::RenderViewHostTestHarness,
+      public testing::WithParamInterface<bool> {
  public:
   InterceptNavigationThrottleTest()
-      : mock_callback_receiver_(new MockInterceptCallbackReceiver()) {}
+      : mock_callback_receiver_(new MockInterceptCallbackReceiver()) {
+    if (GetParam()) {
+      scoped_feature_.InitAndEnableFeature(
+          InterceptNavigationThrottle::kAsyncCheck);
+    } else {
+      scoped_feature_.InitAndDisableFeature(
+          InterceptNavigationThrottle::kAsyncCheck);
+    }
+  }
 
   std::unique_ptr<content::NavigationThrottle> CreateThrottle(
       content::NavigationHandle* handle) {
@@ -105,11 +115,12 @@ class InterceptNavigationThrottleTest
     return simulator->GetLastThrottleCheckResult();
   }
 
+  base::test::ScopedFeatureList scoped_feature_;
   std::unique_ptr<MockInterceptCallbackReceiver> mock_callback_receiver_;
 };
 
-TEST_F(InterceptNavigationThrottleTest,
-       RequestDeferredAndResumedIfNavigationNotIgnored) {
+TEST_P(InterceptNavigationThrottleTest,
+       RequestCompletesIfNavigationNotIgnored) {
   ON_CALL(*mock_callback_receiver_, ShouldIgnoreNavigation(_, _))
       .WillByDefault(Return(false));
   EXPECT_CALL(
@@ -121,8 +132,7 @@ TEST_F(InterceptNavigationThrottleTest,
   EXPECT_EQ(NavigationThrottle::PROCEED, result);
 }
 
-TEST_F(InterceptNavigationThrottleTest,
-       RequestDeferredAndCancelledIfNavigationIgnored) {
+TEST_P(InterceptNavigationThrottleTest, RequestCancelledIfNavigationIgnored) {
   ON_CALL(*mock_callback_receiver_, ShouldIgnoreNavigation(_, _))
       .WillByDefault(Return(true));
   EXPECT_CALL(
@@ -134,7 +144,7 @@ TEST_F(InterceptNavigationThrottleTest,
   EXPECT_EQ(NavigationThrottle::CANCEL_AND_IGNORE, result);
 }
 
-TEST_F(InterceptNavigationThrottleTest, CallbackIsPostFalseForGet) {
+TEST_P(InterceptNavigationThrottleTest, CallbackIsPostFalseForGet) {
   EXPECT_CALL(*mock_callback_receiver_,
               ShouldIgnoreNavigation(
                   _, AllOf(NavigationParamsUrlIsTest(),
@@ -147,7 +157,7 @@ TEST_F(InterceptNavigationThrottleTest, CallbackIsPostFalseForGet) {
   EXPECT_EQ(NavigationThrottle::PROCEED, result);
 }
 
-TEST_F(InterceptNavigationThrottleTest, CallbackIsPostTrueForPost) {
+TEST_P(InterceptNavigationThrottleTest, CallbackIsPostTrueForPost) {
   EXPECT_CALL(*mock_callback_receiver_,
               ShouldIgnoreNavigation(
                   _, AllOf(NavigationParamsUrlIsTest(),
@@ -159,7 +169,7 @@ TEST_F(InterceptNavigationThrottleTest, CallbackIsPostTrueForPost) {
   EXPECT_EQ(NavigationThrottle::PROCEED, result);
 }
 
-TEST_F(InterceptNavigationThrottleTest,
+TEST_P(InterceptNavigationThrottleTest,
        CallbackIsPostFalseForPostConvertedToGetBy302) {
   EXPECT_CALL(*mock_callback_receiver_,
               ShouldIgnoreNavigation(
@@ -178,13 +188,9 @@ TEST_F(InterceptNavigationThrottleTest,
 }
 
 // Ensure POST navigations are cancelled before the start.
-TEST_F(InterceptNavigationThrottleTest, PostNavigationCancelledAtStart) {
-  EXPECT_CALL(*mock_callback_receiver_,
-              ShouldIgnoreNavigation(
-                  _, AllOf(NavigationParamsUrlIsTest(),
-                           Property(&NavigationParams::is_post, Eq(true)))))
-      .WillOnce(Return(true));
-
+TEST_P(InterceptNavigationThrottleTest, PostNavigationCancelledAtStart) {
+  ON_CALL(*mock_callback_receiver_, ShouldIgnoreNavigation(_, _))
+      .WillByDefault(Return(true));
   auto throttle_inserter = CreateThrottleInserter();
   std::unique_ptr<content::NavigationSimulator> simulator =
       content::NavigationSimulator::CreateRendererInitiated(GURL(kTestUrl),
@@ -195,4 +201,8 @@ TEST_F(InterceptNavigationThrottleTest, PostNavigationCancelledAtStart) {
   EXPECT_EQ(NavigationThrottle::CANCEL_AND_IGNORE, result);
 }
 
+INSTANTIATE_TEST_CASE_P(,
+                        InterceptNavigationThrottleTest,
+                        testing::Values(true, false));
+
 }  // namespace navigation_interception

tools/metrics/histograms/histograms.xml

@@ -45327,6 +45327,14 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="Navigation.Intercept.Ignored" enum="Boolean">
+  <owner>csharrison@chromium.org</owner>
+  <summary>
+    Records whether the InterceptNavigationThrottle ignored the navigation. This
+    is recorded at the end of every navigation the throttle observes.
+  </summary>
+</histogram>
+
 <histogram name="Navigation.Intercept.WillStart" units="microseconds">
   <owner>csharrison@chromium.org</owner>
   <summary>