[PageLoadMetrics] Don't record an ads histogram if the page hasn't committed

chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc

Index: chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
diff --git a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
index 2d085ad371a7bb731f13f20d34c7413770ec8e3d..7b7729a39b3b2f14cea8e41672aa5e1c10975dfc 100644
--- a/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
+++ b/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_unittest.cc
@@ -8,11 +8,16 @@
 
 #include "base/macros.h"
 #include "base/test/histogram_tester.h"
+#include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"
 #include "chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h"
 #include "chrome/browser/page_load_metrics/page_load_metrics_observer.h"
 #include "chrome/browser/page_load_metrics/page_load_tracker.h"
+#include "content/public/browser/global_request_id.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/resource_type.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
@@ -34,6 +39,76 @@ const char kNonAdUrl2[] = "https://bar.com/";
 const char kAdName[] = "google_ads_iframe_1";
 const char kNonAdName[] = "foo";
 
+class DelayWillProcessResponseObserver;
+
+// TODO(csharrison): Add this to the content public test API if we find that
+// this is useful in other places.
+// Delays WillProcessResponse until the caller tells it to cancel.
+class DelayWillProcessResponseThrottle : public content::NavigationThrottle {
+ public:
+  explicit DelayWillProcessResponseThrottle(
+      content::NavigationHandle* navigation_handle)
+      : NavigationThrottle(navigation_handle) {}
+
+  // content::NavigationThrottle:
+  ThrottleCheckResult WillProcessResponse() override {
+    return NavigationThrottle::DEFER;
+  }
+  const char* GetNameForLogging() override {
+    return "DelayWillProcessResponseThrottle";
+  }
+
+  void CancelDeferredNavigation() {
+    navigation_handle()->CancelDeferredNavigation(NavigationThrottle::CANCEL);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DelayWillProcessResponseThrottle);
+};
+
+// Adds a navigation throttle to the navigation which delays
+// WillProcessResponse.
+class DelayWillProcessResponseObserver : public content::WebContentsObserver {
+ public:
+  explicit DelayWillProcessResponseObserver(content::WebContents* contents)
+      : content::WebContentsObserver(contents) {}
+
+  // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    std::unique_ptr<content::NavigationThrottle> delay_throttle =
+        base::MakeUnique<DelayWillProcessResponseThrottle>(navigation_handle);
+    throttle_ =
+        static_cast<DelayWillProcessResponseThrottle*>(delay_throttle.get());
+    navigation_handle->RegisterThrottleForTesting(std::move(delay_throttle));
+  }
+
+  void LoadResourceForMainFrameAndResume(
+      page_load_metrics::MetricsWebContentsObserver* observer) {
+    DCHECK(throttle_);
+
+    // Load a resource for the main frame before it commits.
+    content::NavigationHandle* navigation_handle =
+        throttle_->navigation_handle();
+
+    observer->OnRequestComplete(
+        GURL(kNonAdUrl),
+        navigation_handle->GetRenderFrameHost()->GetFrameTreeNodeId(),
+        navigation_handle->GetGlobalRequestID(),
+        content::RESOURCE_TYPE_MAIN_FRAME, false /* was_cached */,
+        nullptr /* data_reduction_proxy */, 10 * 1024 /* raw_body_bytes */,
+        0 /* original_network_content_length */, base::TimeTicks::Now());
+
+    throttle_->CancelDeferredNavigation();
+  }
+
+ private:
+  DelayWillProcessResponseThrottle* throttle_ = nullptr;
+  content::GlobalRequestID global_request_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(DelayWillProcessResponseObserver);
+};
+
 }  // namespace
 
 class AdsPageLoadMetricsObserverTest
@@ -605,3 +680,32 @@ TEST_F(AdsPageLoadMetricsObserverTest, FrameWithNoParent) {
       "PageLoad.Clients.Ads.Google.ResourceTypeWhenNoFrameFound",
       content::RESOURCE_TYPE_SUB_FRAME, 1);
 }
+
+// Make sure that ads histograms aren't recorded if the tracker never commits
+// (see https://crbug.com/723219).
+TEST_F(AdsPageLoadMetricsObserverTest, NoHistogramWithoutCommit) {
+  // Once the metrics observer has the GlobalRequestID, throttle.
+  DelayWillProcessResponseObserver delay_observer(web_contents());
+
+  // Start main-frame navigation
+  auto navigation_simulator = NavigationSimulator::CreateRendererInitiated(
+      GURL(kNonAdUrl), web_contents()->GetMainFrame());
+  navigation_simulator->Start();
+
+  // This will be run once WillProcessResponse defers and the navigation
+  // simulator runs the message loop waiting for the throttles to finish.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &DelayWillProcessResponseObserver::LoadResourceForMainFrameAndResume,
+          base::Unretained(&delay_observer), observer()));
+
+  // The commit will defer after calling WillProcessNavigationResponse, it
+  // will load a resource, and then the throttle will cancel the commit.
+  navigation_simulator->Commit();
+
+  // There shouldn't be any histograms for an aborted main frame.
+  EXPECT_EQ(0u, histogram_tester()
+                    .GetTotalCountsForPrefix("PageLoad.Clients.Ads.")
+                    .size());
+}