Record Isolated Prerender AfterSRPClick event on failed navigations

Refactors the computation of the AfterSRPMetrics class into a separate
method so that it can be called by the PLM Observer when a navigation
fails to commit.

Bug: 1106957
Change-Id: Iaf9cc055f16733f491a1408d6fbe1f4f808a8bd2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343485
Commit-Queue: Robert Ogden <robertogden@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796099}

chrome/browser/page_load_metrics/observers/isolated_prerender_page_load_metrics_observer.cc

@@ -127,6 +127,28 @@ IsolatedPrerenderPageLoadMetricsObserver::OnCommit(
   return CONTINUE_OBSERVING;
 }
 
+void IsolatedPrerenderPageLoadMetricsObserver::OnDidInternalNavigationAbort(
+    content::NavigationHandle* navigation_handle) {
+  IsolatedPrerenderTabHelper* tab_helper =
+      IsolatedPrerenderTabHelper::FromWebContents(
+          navigation_handle->GetWebContents());
+  if (!tab_helper)
+    return;
+
+  std::unique_ptr<IsolatedPrerenderTabHelper::AfterSRPMetrics>
+      after_srp_metrics =
+          tab_helper->ComputeAfterSRPMetricsBeforeCommit(navigation_handle);
+  if (!after_srp_metrics)
+    return;
+
+  // Metrics should also be recorded when the navigation failed due to an abort
+  // or otherwise. That way, we don't skew the metrics towards only pages that
+  // commit.
+  after_srp_metrics_ = *after_srp_metrics;
+
+  RecordAfterSRPEvent();
+}
+
 void IsolatedPrerenderPageLoadMetricsObserver::OnEventOccurred(
     const void* const event_key) {
   if (event_key == IsolatedPrerenderTabHelper::PrefetchingLikelyEventKey()) {

chrome/browser/page_load_metrics/observers/isolated_prerender_page_load_metrics_observer.h

@@ -71,6 +71,8 @@ class IsolatedPrerenderPageLoadMetricsObserver
       content::NavigationHandle* navigation_handle) override;
   ObservePolicy OnCommit(content::NavigationHandle* navigation_handle,
                          ukm::SourceId source_id) override;
+  void OnDidInternalNavigationAbort(
+      content::NavigationHandle* navigation_handle) override;
   ObservePolicy FlushMetricsOnAppEnterBackground(
       const page_load_metrics::mojom::PageLoadTiming& timing) override;
   ObservePolicy OnHidden(

chrome/browser/prerender/isolated/isolated_prerender_browsertest.cc

@@ -1213,6 +1213,71 @@ IN_PROC_BROWSER_TEST_F(IsolatedPrerenderBrowserTest,
           ukm::builders::PrefetchProxy_AfterSRPClick::kProbeLatencyMsName));
 }
 
+// 204's don't commit so this is used to test that the AfterSRPMetrics UKM event
+// is recorded if the page does not commit. In the wild, we expect this to
+// normally occur due to aborted navigations but the end result is the same.
+IN_PROC_BROWSER_TEST_F(IsolatedPrerenderBrowserTest,
+                       DISABLE_ON_WIN_MAC_CHROMEOS(PrefetchingUKM_NoCommit)) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      "isolated-prerender-unlimited-prefetches");
+
+  GURL starting_page = GetOriginServerURL("/simple.html");
+  SetDataSaverEnabled(true);
+  ui_test_utils::NavigateToURL(browser(), starting_page);
+  WaitForUpdatedCustomProxyConfig();
+
+  IsolatedPrerenderTabHelper* tab_helper =
+      IsolatedPrerenderTabHelper::FromWebContents(GetWebContents());
+
+  GURL eligible_link_204 =
+      GetOriginServerURL("/prerender/isolated/page204.html");
+
+  TestTabHelperObserver tab_helper_observer(tab_helper);
+  tab_helper_observer.SetExpectedSuccessfulURLs({eligible_link_204});
+
+  base::RunLoop run_loop;
+  tab_helper_observer.SetOnPrefetchSuccessfulClosure(run_loop.QuitClosure());
+
+  base::HistogramTester histogram_tester;
+
+  GURL doc_url("https://www.google.com/search?q=test");
+  MakeNavigationPrediction(doc_url, {eligible_link_204});
+
+  // This run loop will quit when all the prefetch responses have been
+  // successfully done and processed.
+  run_loop.Run();
+
+  histogram_tester.ExpectUniqueSample(
+      "IsolatedPrerender.Prefetch.Mainframe.RespCode", 204, 1);
+
+  // Navigate to a prefetched page to trigger UKM recording. Note that because
+  // the navigation is never committed, the UKM recording happens immediately.
+  ui_test_utils::NavigateToURL(browser(), eligible_link_204);
+  base::RunLoop().RunUntilIdle();
+
+  VerifyUKMAfterSRP(
+      eligible_link_204,
+      ukm::builders::PrefetchProxy_AfterSRPClick::kClickedLinkSRPPositionName,
+      0);
+  VerifyUKMAfterSRP(
+      eligible_link_204,
+      ukm::builders::PrefetchProxy_AfterSRPClick::kSRPPrefetchEligibleCountName,
+      1);
+  // 0 is the value of |PrefetchStatus::kPrefetchUsedNoProbe|. The enum is not
+  // used here intentionally because its value should never change.
+  VerifyUKMAfterSRP(
+      eligible_link_204,
+      ukm::builders::PrefetchProxy_AfterSRPClick::kSRPClickPrefetchStatusName,
+      0);
+
+  EXPECT_EQ(
+      base::nullopt,
+      GetUKMMetric(
+          eligible_link_204,
+          ukm::builders::PrefetchProxy_AfterSRPClick::kEntryName,
+          ukm::builders::PrefetchProxy_AfterSRPClick::kProbeLatencyMsName));
+}
+
 IN_PROC_BROWSER_TEST_F(
     IsolatedPrerenderBrowserTest,
     DISABLE_ON_WIN_MAC_CHROMEOS(PrefetchingUKM_PrefetchError)) {

chrome/browser/prerender/isolated/isolated_prerender_tab_helper.cc

@@ -413,6 +413,55 @@ IsolatedPrerenderTabHelper::MaybeUpdatePrefetchStatusWithNSPContext(
   return status;
 }
 
+std::unique_ptr<IsolatedPrerenderTabHelper::AfterSRPMetrics>
+IsolatedPrerenderTabHelper::ComputeAfterSRPMetricsBeforeCommit(
+    content::NavigationHandle* handle) const {
+  if (page_->srp_metrics_->predicted_urls_count_ <= 0) {
+    return nullptr;
+  }
+
+  auto metrics = std::make_unique<AfterSRPMetrics>();
+
+  metrics->url_ = handle->GetURL();
+  metrics->prefetch_eligible_count_ =
+      page_->srp_metrics_->prefetch_eligible_count_;
+
+  metrics->probe_latency_ = page_->probe_latency_;
+
+  // Check every url in the redirect chain for a status, starting at the end
+  // and working backwards. Note: When a redirect chain is eligible all the
+  // way to the end, the status is already propagated. But if a redirect was
+  // not eligible then this will find its last known status.
+  DCHECK(!handle->GetRedirectChain().empty());
+  base::Optional<PrefetchStatus> status;
+  base::Optional<size_t> prediction_position;
+  for (auto back_iter = handle->GetRedirectChain().rbegin();
+       back_iter != handle->GetRedirectChain().rend(); ++back_iter) {
+    GURL chain_url = *back_iter;
+    auto status_iter = page_->prefetch_status_by_url_.find(chain_url);
+    if (!status && status_iter != page_->prefetch_status_by_url_.end()) {
+      status = MaybeUpdatePrefetchStatusWithNSPContext(chain_url,
+                                                       status_iter->second);
+    }
+
+    // Same check for the original prediction ordering.
+    auto position_iter = page_->original_prediction_ordering_.find(chain_url);
+    if (!prediction_position &&
+        position_iter != page_->original_prediction_ordering_.end()) {
+      prediction_position = position_iter->second;
+    }
+  }
+
+  if (status) {
+    metrics->prefetch_status_ = *status;
+  } else {
+    metrics->prefetch_status_ = PrefetchStatus::kNavigatedToLinkNotOnSRP;
+  }
+  metrics->clicked_link_srp_position_ = prediction_position;
+
+  return metrics;
+}
+
 void IsolatedPrerenderTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -450,46 +499,8 @@ void IsolatedPrerenderTabHelper::DidFinishNavigation(
     // If the previous page load was a Google SRP, the AfterSRPMetrics class
     // needs to be created now from the SRP's |page_| and then set on the new
     // one when we set it at the end of this method.
-    new_page->after_srp_metrics_ = std::make_unique<AfterSRPMetrics>();
-    new_page->after_srp_metrics_->url_ = url;
-    new_page->after_srp_metrics_->prefetch_eligible_count_ =
-        page_->srp_metrics_->prefetch_eligible_count_;
-
-    new_page->after_srp_metrics_->probe_latency_ = page_->probe_latency_;
-
-    // Check every url in the redirect chain for a status, starting at the end
-    // and working backwards. Note: When a redirect chain is eligible all the
-    // way to the end, the status is already propagated. But if a redirect was
-    // not eligible then this will find its last known status.
-    DCHECK(!navigation_handle->GetRedirectChain().empty());
-    base::Optional<PrefetchStatus> status;
-    base::Optional<size_t> prediction_position;
-    for (auto back_iter = navigation_handle->GetRedirectChain().rbegin();
-         back_iter != navigation_handle->GetRedirectChain().rend();
-         ++back_iter) {
-      GURL chain_url = *back_iter;
-      auto status_iter = page_->prefetch_status_by_url_.find(chain_url);
-      if (!status && status_iter != page_->prefetch_status_by_url_.end()) {
-        status = MaybeUpdatePrefetchStatusWithNSPContext(chain_url,
-                                                         status_iter->second);
-      }
-
-      // Same check for the original prediction ordering.
-      auto position_iter = page_->original_prediction_ordering_.find(chain_url);
-      if (!prediction_position &&
-          position_iter != page_->original_prediction_ordering_.end()) {
-        prediction_position = position_iter->second;
-      }
-    }
-
-    if (status) {
-      new_page->after_srp_metrics_->prefetch_status_ = *status;
-    } else {
-      new_page->after_srp_metrics_->prefetch_status_ =
-          PrefetchStatus::kNavigatedToLinkNotOnSRP;
-    }
-    new_page->after_srp_metrics_->clicked_link_srp_position_ =
-        prediction_position;
+    new_page->after_srp_metrics_ =
+        ComputeAfterSRPMetricsBeforeCommit(navigation_handle);
 
     // See if the page being navigated to was prerendered. If so, copy over its
     // subresource manager and networking pipes.

chrome/browser/prerender/isolated/isolated_prerender_tab_helper.h

@@ -398,6 +398,14 @@ class IsolatedPrerenderTabHelper
     scoped_refptr<network::SharedURLLoaderFactory> isolated_url_loader_factory_;
   };
 
+  // Computes the AfterSRPMetrics that would be returned for the next
+  // navigation, when it commits. This method exists to allow the PLM Observer
+  // to get the AfterSRPMetrics if the navigation fails to commit, so that
+  // metrics can be logged anyways. Returns nullptr if the after srp metrics
+  // wouldn't be set on the next commit.
+  std::unique_ptr<IsolatedPrerenderTabHelper::AfterSRPMetrics>
+  ComputeAfterSRPMetricsBeforeCommit(content::NavigationHandle* handle) const;
+
   // A helper method to make it easier to tell when prefetching is already
   // active.
   bool PrefetchingActive() const;

chrome/browser/prerender/isolated/isolated_prerender_tab_helper_unittest.cc

@@ -1669,14 +1669,14 @@ TEST_F(IsolatedPrerenderTabHelperRedirectTest, NoRedirect_Insecure_Continued) {
   GURL final_url("http://final.com/");
 
   content::MockNavigationHandle handle(web_contents());
-  handle.set_url(final_url);
+  handle.set_url(url);
   tab_helper()->DidStartNavigation(&handle);
   handle.set_has_committed(true);
   handle.set_redirect_chain({
-      GURL("https://start.test.com"),
       url,
       final_url,
   });
+  handle.set_url(final_url);
   tab_helper()->DidFinishNavigation(&handle);
 
   ASSERT_TRUE(tab_helper()->after_srp_metrics().has_value());

chrome/test/data/prerender/isolated/page204.html

+<title>This page intentionally left blank</title>

chrome/test/data/prerender/isolated/page204.html.mock-http-headers

+HTTP/1.0 204 No Content
+Content-Length: 0
+Content-Type: text/html