Reland: Predictor: Dispatch all predictions async from the resource throttle

This patch relands the following CL:
https://chromium-review.googlesource.com/c/chromium/src/+/821090

This is a small refactor that should have some performance wins. Now
that PlzNavigate has launched, we should be dispatching our preconnects
after the initial navigation request has gone out, not before. This is
because frequently (especially on Android), dispatching preconnects
tends to hog the IO thread, delaying the initial request.

Note that this predictor is probably going away, but this CPU work
is in the critical path of some other work I am planning on tackling.

Bug: 792524
Change-Id: I03b058af249088f779dfb574c8552b32150bea2c
Reviewed-on: https://chromium-review.googlesource.com/826783Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524308}

chrome/browser/loader/predictor_resource_throttle.cc

@@ -5,6 +5,9 @@
 #include "chrome/browser/loader/predictor_resource_throttle.h"
 
 #include "base/memory/ptr_util.h"
+#include "base/sequenced_task_runner.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/trace_event/trace_event.h"
 #include "chrome/browser/net/predictor.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "content/public/browser/resource_request_info.h"
@@ -27,7 +30,7 @@ bool IsNavigationRequest(net::URLRequest* request) {
 PredictorResourceThrottle::PredictorResourceThrottle(
     net::URLRequest* request,
     chrome_browser_net::Predictor* predictor)
-    : request_(request), predictor_(predictor) {}
+    : request_(request), predictor_(predictor), weak_factory_(this) {}
 
 PredictorResourceThrottle::~PredictorResourceThrottle() {}
 
@@ -68,8 +71,6 @@ void PredictorResourceThrottle::WillStartRequest(bool* defer) {
     predictor_->LearnFromNavigation(referring_scheme_host, request_scheme_host);
   }
 
-  // Subresources for main frames are predicted when navigation starts, in
-  // PredictorTabHelper, so only handle predictions for subframes here.
   // If the referring host is equal to the request host, then the predictor has
   // already made any/all predictions when navigating to the referring host.
   // Don't update the RecentlySeen() time because the timed cache is already
@@ -77,10 +78,7 @@ void PredictorResourceThrottle::WillStartRequest(bool* defer) {
   if (IsNavigationRequest(request_) &&
       referring_scheme_host != request_scheme_host) {
     predictor_->timed_cache()->SetRecentlySeen(request_scheme_host);
-    if (resource_type == content::RESOURCE_TYPE_SUB_FRAME) {
-      predictor_->PredictFrameSubresources(request_scheme_host,
-                                           request_->site_for_cookies());
-    }
+    DispatchPredictions(request_scheme_host, request_->site_for_cookies());
   }
 }
 
@@ -115,10 +113,26 @@ void PredictorResourceThrottle::WillRedirectRequest(
   }
 
   predictor_->timed_cache()->SetRecentlySeen(new_scheme_host);
-  predictor_->PredictFrameSubresources(new_scheme_host,
-                                       redirect_info.new_site_for_cookies);
+  DispatchPredictions(new_scheme_host, redirect_info.new_site_for_cookies);
 }
 
 const char* PredictorResourceThrottle::GetNameForLogging() const {
   return "PredictorResourceThrottle";
 }
+
+void PredictorResourceThrottle::DispatchPredictions(
+    const GURL& url,
+    const GURL& site_for_cookies) {
+  // Dispatch predictions asynchronously to avoid blocking the actual request
+  // from going out to the network.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&PredictorResourceThrottle::DoPredict,
+                     weak_factory_.GetWeakPtr(), url, site_for_cookies));
+}
+
+void PredictorResourceThrottle::DoPredict(const GURL& url,
+                                          const GURL& site_for_cookies) {
+  TRACE_EVENT0("loading", "PredictorResourceThrottle::DoPredict");
+  predictor_->PredictFrameSubresources(url, site_for_cookies);
+}

chrome/browser/loader/predictor_resource_throttle.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/resource_throttle.h"
 
 namespace chrome_browser_net {
@@ -19,20 +20,19 @@ struct RedirectInfo;
 class URLRequest;
 }
 
+class GURL;
 class ProfileIOData;
 
 // This resource throttle tracks requests in order to help the predictor learn
 // resource relationships. It notifies the predictor of redirect and referrer
 // relationships, and populates the predictor's timed cache of ongoing
-// navigations. It also initiates predictive actions based on subframe
+// navigations. It also initiates predictive actions based on navigation
 // requests and redirects.
 // Note: This class does not issue predictive actions off of the initial main
 // frame request (before any redirects). That is done on the UI thread in
 // response to navigation callbacks in predictor_tab_helper.cc.
 // TODO(csharrison): This class shouldn't depend on chrome_browser_net. The
 // predictor should probably be moved here (along with its dependencies).
-// TODO(csharrison): When the PreconnectMore experiment is over, move all
-// prediction dispatching to the tab helper.
 class PredictorResourceThrottle : public content::ResourceThrottle {
  public:
   PredictorResourceThrottle(net::URLRequest* request,
@@ -50,9 +50,14 @@ class PredictorResourceThrottle : public content::ResourceThrottle {
   const char* GetNameForLogging() const override;
 
  private:
+  void DispatchPredictions(const GURL& url, const GURL& site_for_cookies);
+  void DoPredict(const GURL& url, const GURL& site_for_cookies);
+
   net::URLRequest* request_;
   chrome_browser_net::Predictor* predictor_;
 
+  base::WeakPtrFactory<PredictorResourceThrottle> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(PredictorResourceThrottle);
 };
 

chrome/browser/net/predictor_browsertest.cc

@@ -61,6 +61,7 @@
 #include "net/url_request/url_request_test_job.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "url/gurl.h"
+#include "url/url_constants.h"
 
 using content::BrowserThread;
 using testing::HasSubstr;
@@ -1410,9 +1411,14 @@ IN_PROC_BROWSER_TEST_F(PredictorBrowserTest,
   EXPECT_EQ(0u, cross_site_connection_listener_->GetAcceptedSocketCount());
 
   // Now, navigate using a renderer initiated navigation and expect preconnects.
+  // Need to navigate to about:blank first so the referring site is different
+  // from the request site.
+  ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
   EXPECT_TRUE(content::ExecuteScript(
       browser()->tab_strip_model()->GetActiveWebContents(),
-      "window.location.href='/title1.html'"));
+      base::StringPrintf(
+          "window.location.href='%s'",
+          embedded_test_server()->GetURL("/title1.html").spec().c_str())));
 
   // The renderer initiated navigation is not synchronous, so just wait for the
   // preconnects to go through.

chrome/browser/net/predictor_tab_helper.cc

@@ -6,10 +6,6 @@
 
 #include "chrome/browser/net/predictor.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/common/chrome_features.h"
-#include "chrome/common/url_constants.h"
-#include "content/public/browser/navigation_handle.h"
-#include "content/public/common/browser_side_navigation_policy.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -20,41 +16,9 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(chrome_browser_net::PredictorTabHelper);
 namespace chrome_browser_net {
 
 PredictorTabHelper::PredictorTabHelper(content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents),
-      predicted_from_pending_entry_(false) {
-}
-
-PredictorTabHelper::~PredictorTabHelper() {
-}
+    : content::WebContentsObserver(web_contents) {}
 
-void PredictorTabHelper::DidStartNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (!base::FeatureList::IsEnabled(features::kPreconnectMore) &&
-      (!content::IsBrowserSideNavigationEnabled() ||
-       navigation_handle->IsRendererInitiated()))
-    return;
-  // Subframe navigations are handled in WitnessURLRequest.
-  if (!navigation_handle->IsInMainFrame())
-    return;
-  if (predicted_from_pending_entry_) {
-    predicted_from_pending_entry_ = false;
-    return;
-  }
-  PreconnectUrl(navigation_handle->GetURL());
-}
-
-void PredictorTabHelper::DidStartNavigationToPendingEntry(
-    const GURL& url,
-    content::ReloadType reload_type) {
-  // This method isn't needed with PlzNavigate (see comment in header for
-  // predicted_from_pending_entry_)
-  if (content::IsBrowserSideNavigationEnabled())
-    return;
-
-  // The standard way to preconnect based on navigation.
-  PreconnectUrl(url);
-  predicted_from_pending_entry_ = true;
-}
+PredictorTabHelper::~PredictorTabHelper() {}
 
 void PredictorTabHelper::DocumentOnLoadCompletedInMainFrame() {
   Profile* profile =
@@ -68,12 +32,4 @@ void PredictorTabHelper::DocumentOnLoadCompletedInMainFrame() {
     predictor->SaveStateForNextStartup();
 }
 
-void PredictorTabHelper::PreconnectUrl(const GURL& url) {
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-  Predictor* predictor(profile->GetNetworkPredictor());
-  if (predictor && url.SchemeIsHTTPOrHTTPS())
-    predictor->PreconnectUrlAndSubresources(url, GURL());
-}
-
 }  // namespace chrome_browser_net

chrome/browser/net/predictor_tab_helper.h

@@ -6,16 +6,9 @@
 #define CHROME_BROWSER_NET_PREDICTOR_TAB_HELPER_H_
 
 #include "base/macros.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/reload_type.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
-namespace content {
-class NavigationHandle;
-}
-
 namespace chrome_browser_net {
 
 class PredictorTabHelper
@@ -25,28 +18,12 @@ class PredictorTabHelper
   ~PredictorTabHelper() override;
 
   // content::WebContentsObserver:
-  void DidStartNavigation(
-      content::NavigationHandle* navigation_handle) override;
-  void DidStartNavigationToPendingEntry(
-      const GURL& url,
-      content::ReloadType reload_type) override;
   void DocumentOnLoadCompletedInMainFrame() override;
 
  private:
   explicit PredictorTabHelper(content::WebContents* web_contents);
   friend class content::WebContentsUserData<PredictorTabHelper>;
 
-  void PreconnectUrl(const GURL& url);
-
-  // This boolean is set to true after a call to
-  // DidStartNavigationToPendingEntry, which fires off predictive preconnects.
-  // This ensures that the resulting call to DidStartNavigation does not
-  // duplicate these preconnects. The tab helper spawns preconnects on the
-  // navigation to the pending entry because DidStartNavigation is called after
-  // render process spawn (pre-PlzNavigate), which can take substantial time
-  // especially on Android.
-  bool predicted_from_pending_entry_;
-
   DISALLOW_COPY_AND_ASSIGN(PredictorTabHelper);
 };