[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..087d91dbd94d1c9bffa833f90eeb70bb0f271fef 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,78 @@ const char kNonAdUrl2[] = "https://bar.com/";
 const char kAdName[] = "google_ads_iframe_1";
 const char kNonAdName[] = "foo";
 
+class DelayWillProcessResponseObserver;
+
+// Delays WillProcessResponse until the caller tells it to cancel.

[Charlie Harrison, 2017/05/17 18:54:51]

Would you add a TODO(csharrison) to add this to the content public test API if
another consumer needs it?

[jkarlin, 2017/05/18 13:50:13]

Done.

+class DelayWillProcessResponseThrottle : public content::NavigationThrottle {
+ public:
+  explicit DelayWillProcessResponseThrottle(
+      content::NavigationHandle* navigation_handle)
+      : NavigationThrottle(navigation_handle) {}
+
+  // NavigationThrottle overrides

[Charlie Harrison, 2017/05/17 18:54:50]

  // content::NavigationThrottle:

[jkarlin, 2017/05/18 13:50:13]

Done.

+  ThrottleCheckResult WillProcessResponse() override {
+    return NavigationThrottle::DEFER;
+  }
+  const char* GetNameForLogging() override {
+    return "DelayWillProcessResponseThrottle";
+  }
+
+  void CancelDeferredNavigation() {
+    navigation_handle()->CancelDeferredNavigation(NavigationThrottle::CANCEL);
+  }
+
+  content::GlobalRequestID GetGlobalRequestID() const {
+    return navigation_handle()->GetGlobalRequestID();
+  }
+
+ 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)
+      : WebContentsObserver(contents), throttle_(nullptr) {}

[Charlie Harrison, 2017/05/17 18:54:50]

Hm why don't you need content:: prefix to WebContentsObserver?

[Charlie Harrison, 2017/05/17 18:54:50]

Don't need to set throttle_ to nullptr here since you do it below.

[jkarlin, 2017/05/18 13:50:13]

Done.

[jkarlin, 2017/05/18 13:50:13]

I have no idea. Added for clarity.

+
+  // WebContentsObserver override

[Charlie Harrison, 2017/05/17 18:54:50]

  // content::WebContentsObserver:

[jkarlin, 2017/05/18 13:50:13]

Done.

+  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));
+  }
+
+  DelayWillProcessResponseThrottle* throttle() const { return throttle_; }
+
+ private:
+  DelayWillProcessResponseThrottle* throttle_ = nullptr;
+  content::GlobalRequestID global_request_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(DelayWillProcessResponseObserver);
+};
+
+void LoadResourceForMainFrameAndResume(

[Charlie Harrison, 2017/05/17 18:54:50]

Why not include this as a method in the delay observer so you don't need to
expose the throttle?

[jkarlin, 2017/05/18 13:50:13]

Done.

+    page_load_metrics::MetricsWebContentsObserver* observer,
+    DelayWillProcessResponseThrottle* 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();
+}
+
 }  // namespace
 
 class AdsPageLoadMetricsObserverTest
@@ -605,3 +682,30 @@ 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(&LoadResourceForMainFrameAndResume, observer(),
+                            delay_observer.throttle()));
+
+  // 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());
+}