Refactor NavigationPredictor to use and expose its WebContents

NavigationPredictor is currently tied to a RFH via mojo channels but
the lifetime of a RFH is difficult to reason about, especially during a
navigation to a different origin. This CL moves NavigationPredictor to
a WebContents-centric approach with the added benefits of easier
lifetime management and easier use in dependent code.

Of important note:
* The mojo channels that manage the lifetime of a NavigationPredictor
are still tied to the main frame RFH since that maps to how renderers
actually work.
* A NavigationPredictor can enter a "lame duck" state where it is
observing a WebContents that has been destroyed, but has yet to be
destroyed itself by mojo. Thankfully, this is easily detected since
a WebContentsObserver's web_contents() method returns nullptr when this
occurs. Both places where web_contents() is used are now guarded by
this check.
* Multiple RFHs may belong to one WebContents, but only one
NavigationPredictor will be created for each WebContents because the
RFH must be the single mainframe to construct a NavigationPredictor.

Bug: 1023485, 1041828
Change-Id: I6234225132048c70539395a3d1869602e7b0a0ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2008775
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734090}

chrome/browser/navigation_predictor/navigation_predictor.cc

@@ -23,7 +23,6 @@
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "components/search_engines/template_url_service.h"
 #include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 #include "mojo/public/cpp/bindings/message.h"
@@ -42,14 +41,6 @@ namespace {
 const base::Feature kNavigationPredictorMultiplePrerenders{
     "NavigationPredictorMultiplePrerenders", base::FEATURE_ENABLED_BY_DEFAULT};
 
-bool IsMainFrame(content::RenderFrameHost* rfh) {
-  // Don't use rfh->GetRenderViewHost()->GetMainFrame() here because
-  // RenderViewHost is being deprecated and because in OOPIF,
-  // RenderViewHost::GetMainFrame() returns nullptr for child frames hosted in a
-  // different process from the main frame.
-  return rfh->GetParent() == nullptr;
-}
-
 std::string GetURLWithoutRefParams(const GURL& gurl) {
   url::Replacements<char> replacements;
   replacements.ClearRef();
@@ -121,10 +112,8 @@ struct NavigationPredictor::NavigationScore {
   base::Optional<size_t> score_rank;
 };
 
-NavigationPredictor::NavigationPredictor(
-    content::RenderFrameHost* render_frame_host)
-    : browser_context_(
-          render_frame_host->GetSiteInstance()->GetBrowserContext()),
+NavigationPredictor::NavigationPredictor(content::WebContents* web_contents)
+    : browser_context_(web_contents->GetBrowserContext()),
       ratio_area_scale_(base::GetFieldTrialParamByFeatureAsInt(
           blink::features::kNavigationPredictor,
           "ratio_area_scale",
@@ -210,22 +199,15 @@ NavigationPredictor::NavigationPredictor(
       normalize_navigation_scores_(base::GetFieldTrialParamByFeatureAsBool(
           blink::features::kNavigationPredictor,
           "normalize_scores",
-          true)),
-      render_frame_host_(render_frame_host) {
+          true)) {
   DCHECK(browser_context_);
   DETACH_FROM_SEQUENCE(sequence_checker_);
-  DCHECK(render_frame_host_);
-
-  if (!IsMainFrame(render_frame_host))
-    return;
 
   if (browser_context_->IsOffTheRecord())
     return;
 
   ukm_recorder_ = ukm::UkmRecorder::Get();
 
-  content::WebContents* web_contents =
-      content::WebContents::FromRenderFrameHost(render_frame_host);
   current_visibility_ = web_contents->GetVisibility();
   ukm_source_id_ = web_contents->GetLastCommittedSourceId();
   Observe(web_contents);
@@ -250,9 +232,13 @@ void NavigationPredictor::Create(
   if (render_frame_host->GetParent())
     return;
 
+  content::WebContents* web_contents =
+      content::WebContents::FromRenderFrameHost(render_frame_host);
+  if (!web_contents)
+    return;
+
   mojo::MakeSelfOwnedReceiver(
-      std::make_unique<NavigationPredictor>(render_frame_host),
-      std::move(receiver));
+      std::make_unique<NavigationPredictor>(web_contents), std::move(receiver));
 }
 
 bool NavigationPredictor::IsValidMetricFromRenderer(
@@ -261,7 +247,6 @@ bool NavigationPredictor::IsValidMetricFromRenderer(
          metric.source_url.SchemeIsHTTPOrHTTPS();
 }
 
-
 void NavigationPredictor::RecordTimingOnClick() {
   base::TimeTicks current_timing = base::TimeTicks::Now();
 
@@ -948,6 +933,13 @@ double NavigationPredictor::GetPageMetricsScore() const {
 void NavigationPredictor::NotifyPredictionUpdated(
     const std::vector<std::unique_ptr<NavigationScore>>&
         sorted_navigation_scores) {
+  // It is possible for this class to still exist while its WebContents and
+  // RenderFrameHost are being destroyed. This can be detected by checking
+  // |web_contents()| which will be nullptr if the WebContents has been
+  // destroyed.
+  if (!web_contents())
+    return;
+
   NavigationPredictorKeyedService* service =
       NavigationPredictorKeyedServiceFactory::GetForProfile(
           Profile::FromBrowserContext(browser_context_));
@@ -957,7 +949,7 @@ void NavigationPredictor::NotifyPredictionUpdated(
   for (const auto& nav_score : sorted_navigation_scores) {
     top_urls.push_back(nav_score->url);
   }
-  service->OnPredictionUpdated(render_frame_host_, document_url_, top_urls);
+  service->OnPredictionUpdated(web_contents(), document_url_, top_urls);
 }
 
 void NavigationPredictor::MaybeTakeActionOnLoad(
@@ -1008,6 +1000,13 @@ void NavigationPredictor::Prefetch(
   DCHECK(!prerender_handle_);
   DCHECK(!prefetch_url_);
 
+  // It is possible for this class to still exist while its WebContents and
+  // RenderFrameHost are being destroyed. This can be detected by checking
+  // |web_contents()| which will be nullptr if the WebContents has been
+  // destroyed.
+  if (!web_contents())
+    return;
+
   content::SessionStorageNamespace* session_storage_namespace =
       web_contents()->GetController().GetDefaultSessionStorageNamespace();
   gfx::Size size = web_contents()->GetContainerBounds().size();

chrome/browser/navigation_predictor/navigation_predictor.h

@@ -29,7 +29,7 @@ namespace content {
 class BrowserContext;
 class NavigationHandle;
 class RenderFrameHost;
-}
+}  // namespace content
 
 namespace prerender {
 class PrerenderManager;
@@ -44,9 +44,7 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
                             public content::WebContentsObserver,
                             public prerender::PrerenderHandle::Observer {
  public:
-  // |render_frame_host| is the host associated with the render frame. It is
-  // used to retrieve metrics at the browser side.
-  explicit NavigationPredictor(content::RenderFrameHost* render_frame_host);
+  explicit NavigationPredictor(content::WebContents* web_contents);
   ~NavigationPredictor() override;
 
   // Create and bind NavigationPredictor.
@@ -327,8 +325,8 @@ class NavigationPredictor : public blink::mojom::AnchorElementMetricsHost,
   // The URL of the current page.
   GURL document_url_;
 
-  // Render frame host of the current page.
-  const content::RenderFrameHost* render_frame_host_;
+  // WebContents of the current page.
+  const content::WebContents* web_contents_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 

chrome/browser/navigation_predictor/navigation_predictor_keyed_service.cc

@@ -7,17 +7,15 @@
 #include "base/compiler_specific.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
 
 NavigationPredictorKeyedService::Prediction::Prediction(
-    const content::RenderFrameHost* render_frame_host,
+    const content::WebContents* web_contents,
     const GURL& source_document_url,
     const std::vector<GURL>& sorted_predicted_urls)
-    : render_frame_host_(render_frame_host),
+    : web_contents_(web_contents),
       source_document_url_(source_document_url),
-      sorted_predicted_urls_(sorted_predicted_urls) {
-  // |render_frame_host_| will be used by consumers in future.
-  ALLOW_UNUSED_LOCAL(render_frame_host_);
-}
+      sorted_predicted_urls_(sorted_predicted_urls) {}
 
 NavigationPredictorKeyedService::Prediction::Prediction(
     const NavigationPredictorKeyedService::Prediction& other) = default;
@@ -37,6 +35,11 @@ NavigationPredictorKeyedService::Prediction::sorted_predicted_urls() const {
   return sorted_predicted_urls_;
 }
 
+const content::WebContents*
+NavigationPredictorKeyedService::Prediction::web_contents() const {
+  return web_contents_;
+}
+
 NavigationPredictorKeyedService::NavigationPredictorKeyedService(
     content::BrowserContext* browser_context)
     : search_engine_preconnector_(browser_context) {
@@ -52,12 +55,12 @@ NavigationPredictorKeyedService::~NavigationPredictorKeyedService() {
 }
 
 void NavigationPredictorKeyedService::OnPredictionUpdated(
-    const content::RenderFrameHost* render_frame_host,
+    const content::WebContents* web_contents,
     const GURL& document_url,
     const std::vector<GURL>& sorted_predicted_urls) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   last_prediction_ =
-      Prediction(render_frame_host, document_url, sorted_predicted_urls);
+      Prediction(web_contents, document_url, sorted_predicted_urls);
   for (auto& observer : observer_list_) {
     observer.OnPredictionUpdated(last_prediction_);
   }

chrome/browser/navigation_predictor/navigation_predictor_keyed_service.h

@@ -17,7 +17,7 @@
 
 namespace content {
 class BrowserContext;
-class RenderFrameHost;
+class WebContents;
 }  // namespace content
 
 // Keyed service that can be used to receive notifications about the URLs for
@@ -27,7 +27,7 @@ class NavigationPredictorKeyedService : public KeyedService {
   // Stores the next set of URLs that the user is expected to navigate to.
   class Prediction {
    public:
-    Prediction(const content::RenderFrameHost* render_frame_host,
+    Prediction(const content::WebContents* web_contents,
                const GURL& source_document_url,
                const std::vector<GURL>& sorted_predicted_urls);
     Prediction(const Prediction& other);
@@ -35,11 +35,13 @@ class NavigationPredictorKeyedService : public KeyedService {
     ~Prediction();
     GURL source_document_url() const;
     std::vector<GURL> sorted_predicted_urls() const;
+    const content::WebContents* web_contents() const;
 
    private:
-    // |render_frame_host_| from where the navigation may happen. May be
-    // nullptr.
-    const content::RenderFrameHost* render_frame_host_;
+    // The WebContents from where the navigation may happen. Do not use this
+    // pointer outside the observer's call stack unless its destruction is also
+    // observed.
+    const content::WebContents* web_contents_;
 
     // Current URL of the document from where the navigtion may happen.
     GURL source_document_url_;
@@ -72,7 +74,7 @@ class NavigationPredictorKeyedService : public KeyedService {
   SearchEnginePreconnector* SearchEnginePreconnectorForTesting();
 
   // |document_url| may be invalid. Called by navigation predictor.
-  void OnPredictionUpdated(const content::RenderFrameHost* render_frame_host,
+  void OnPredictionUpdated(const content::WebContents* web_contents,
                            const GURL& document_url,
                            const std::vector<GURL>& sorted_predicted_urls);
 

chrome/browser/navigation_predictor/navigation_predictor_unittest.cc

@@ -33,9 +33,9 @@ class TestNavigationPredictor : public NavigationPredictor {
  public:
   TestNavigationPredictor(
       mojo::PendingReceiver<AnchorElementMetricsHost> receiver,
-      content::RenderFrameHost* render_frame_host,
+      content::WebContents* web_contents,
       bool init_feature_list)
-      : NavigationPredictor(render_frame_host),
+      : NavigationPredictor(web_contents),
         receiver_(this, std::move(receiver)) {
     if (init_feature_list) {
       const std::vector<base::Feature> features = {
@@ -83,10 +83,10 @@ class TestNavigationPredictorBasedOnScroll : public TestNavigationPredictor {
  public:
   TestNavigationPredictorBasedOnScroll(
       mojo::PendingReceiver<AnchorElementMetricsHost> receiver,
-      content::RenderFrameHost* render_frame_host,
+      content::WebContents* web_contents,
       bool init_feature_list)
       : TestNavigationPredictor(std::move(receiver),
-                                render_frame_host,
+                                web_contents,
                                 init_feature_list) {}
 
   ~TestNavigationPredictorBasedOnScroll() override {}
@@ -139,7 +139,7 @@ class NavigationPredictorTest : public ChromeRenderViewHostTestHarness {
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     predictor_service_helper_ = std::make_unique<TestNavigationPredictor>(
-        predictor_service_.BindNewPipeAndPassReceiver(), main_rfh(),
+        predictor_service_.BindNewPipeAndPassReceiver(), web_contents(),
         !field_trial_initiated_);
   }
 
@@ -509,7 +509,7 @@ class NavigationPredictorSendUkmMetricsEnabledTest
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     predictor_service_helper_ = std::make_unique<TestNavigationPredictor>(
-        predictor_service_.BindNewPipeAndPassReceiver(), main_rfh(), false);
+        predictor_service_.BindNewPipeAndPassReceiver(), web_contents(), false);
   }
 
   struct TestMetrics {
@@ -744,7 +744,8 @@ class NavigationPredictorPrefetchAfterPreconnectEnabledTest
     ChromeRenderViewHostTestHarness::SetUp();
     predictor_service_helper_ =
         std::make_unique<TestNavigationPredictorBasedOnScroll>(
-            predictor_service_.BindNewPipeAndPassReceiver(), main_rfh(), false);
+            predictor_service_.BindNewPipeAndPassReceiver(), web_contents(),
+            false);
   }
 
   blink::mojom::AnchorElementMetricsPtr CreateMetricsPtrWithRatioDistance(
@@ -803,7 +804,7 @@ class NavigationPredictorPrefetchDisabledTest : public NavigationPredictorTest {
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     predictor_service_helper_ = std::make_unique<TestNavigationPredictor>(
-        predictor_service_.BindNewPipeAndPassReceiver(), main_rfh(), false);
+        predictor_service_.BindNewPipeAndPassReceiver(), web_contents(), false);
   }
 };
 
@@ -886,7 +887,7 @@ class NavigationPredictorPreconnectPrefetchDisabledTest
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     predictor_service_helper_ = std::make_unique<TestNavigationPredictor>(
-        predictor_service_.BindNewPipeAndPassReceiver(), main_rfh(), false);
+        predictor_service_.BindNewPipeAndPassReceiver(), web_contents(), false);
   }
 };