Make PageLoadMetricsWaiter more resilient

chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"
 
 #include <memory>
 #include <vector>
 
 #include "base/macros.h"
@@ skipping 20 matching lines @@
 namespace {
 
 const char kDefaultTestUrl[] = "https://google.com/";
 const char kDefaultTestUrlAnchor[] = "https://google.com/#samedocument";
 const char kDefaultTestUrl2[] = "https://whatever.com/";
 const char kFilteredStartUrl[] = "https://whatever.com/ignore-on-start";
 const char kFilteredCommitUrl[] = "https://whatever.com/ignore-on-commit";
 
 // Simple PageLoadMetricsObserver that copies observed PageLoadTimings into the
 // provided std::vector, so they can be analyzed by unit tests.
-class TestPageLoadMetricsObserver
-    : public PageLoadMetricsObserver,
-      public MetricsWebContentsObserver::TestingObserver {
+class TestPageLoadMetricsObserver : public PageLoadMetricsObserver {
  public:
   TestPageLoadMetricsObserver(
-      content::WebContents* web_contents,
       std::vector<mojom::PageLoadTimingPtr>* updated_timings,
       std::vector<mojom::PageLoadTimingPtr>* updated_subframe_timings,
       std::vector<mojom::PageLoadTimingPtr>* complete_timings,
       std::vector<GURL>* observed_committed_urls)
-      : MetricsWebContentsObserver::TestingObserver(web_contents),
-        updated_timings_(updated_timings),
+      : updated_timings_(updated_timings),
         updated_subframe_timings_(updated_subframe_timings),
         complete_timings_(complete_timings),
         observed_committed_urls_(observed_committed_urls) {}
 
   ObservePolicy OnStart(content::NavigationHandle* navigation_handle,
                const GURL& currently_committed_url,
                bool started_in_foreground) override {
     observed_committed_urls_->push_back(currently_committed_url);
     return CONTINUE_OBSERVING;
   }
 
-  void OnTimingUpdate(const mojom::PageLoadTiming& timing,
+  void OnTimingUpdate(bool is_subframe,
+                      const mojom::PageLoadTiming& timing,
                       const PageLoadExtraInfo& extra_info) override {
-    updated_timings_->push_back(timing.Clone());
-  }
-
-  void OnTimingUpdated(bool is_main_frame,
-                       const mojom::PageLoadTiming& timing,
-                       const mojom::PageLoadMetadata& metadata) override {
-    if (!is_main_frame) {
+    if (is_subframe)
       updated_subframe_timings_->push_back(timing.Clone());
-    }
+    else
+      updated_timings_->push_back(timing.Clone());
   }
 
   void OnComplete(const mojom::PageLoadTiming& timing,
                   const PageLoadExtraInfo& extra_info) override {
     complete_timings_->push_back(timing.Clone());
   }
 
   ObservePolicy FlushMetricsOnAppEnterBackground(
       const mojom::PageLoadTiming& timing,
       const PageLoadExtraInfo& extra_info) override {
@@ skipping 34 matching lines @@
     completed_filtered_urls_->push_back(extra_info.url);
   }
 
  private:
   std::vector<GURL>* const completed_filtered_urls_;
 };
 
 class TestPageLoadMetricsEmbedderInterface
     : public PageLoadMetricsEmbedderInterface {
  public:
-  explicit TestPageLoadMetricsEmbedderInterface(
-      content::WebContents* web_contents)
-      : web_contents_(web_contents), is_ntp_(false) {}
+  TestPageLoadMetricsEmbedderInterface() : is_ntp_(false) {}
 
   bool IsNewTabPageUrl(const GURL& url) override { return is_ntp_; }
   void set_is_ntp(bool is_ntp) { is_ntp_ = is_ntp; }
   void RegisterObservers(PageLoadTracker* tracker) override {
     tracker->AddObserver(base::MakeUnique<TestPageLoadMetricsObserver>(
-        web_contents_, &updated_timings_, &updated_subframe_timings_,
-        &complete_timings_, &observed_committed_urls_));
+        &updated_timings_, &updated_subframe_timings_, &complete_timings_,
+        &observed_committed_urls_));
     tracker->AddObserver(base::MakeUnique<FilteringPageLoadMetricsObserver>(
         &completed_filtered_urls_));
   }
   const std::vector<mojom::PageLoadTimingPtr>& updated_timings() const {
     return updated_timings_;
   }
   const std::vector<mojom::PageLoadTimingPtr>& complete_timings() const {
     return complete_timings_;
   }
   const std::vector<mojom::PageLoadTimingPtr>& updated_subframe_timings()
@@ skipping 10 matching lines @@
   const std::vector<GURL>& completed_filtered_urls() const {
     return completed_filtered_urls_;
   }
 
  private:
   std::vector<mojom::PageLoadTimingPtr> updated_timings_;
   std::vector<mojom::PageLoadTimingPtr> updated_subframe_timings_;
   std::vector<mojom::PageLoadTimingPtr> complete_timings_;
   std::vector<GURL> observed_committed_urls_;
   std::vector<GURL> completed_filtered_urls_;
-  content::WebContents* web_contents_;
   bool is_ntp_;
 };
 
 }  //  namespace
 
 class MetricsWebContentsObserverTest : public ChromeRenderViewHostTestHarness {
  public:
   MetricsWebContentsObserverTest() : num_errors_(0) {}
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     AttachObserver();
   }
 
   void NavigateToUntrackedUrl() {
     content::WebContentsTester::For(web_contents())
         ->NavigateAndCommit(GURL(url::kAboutBlankURL));
   }
 
   void SimulateTimingUpdate(const mojom::PageLoadTiming& timing) {
     SimulateTimingUpdate(timing, web_contents()->GetMainFrame());
   }
 
   void SimulateTimingUpdate(const mojom::PageLoadTiming& timing,
                             content::RenderFrameHost* render_frame_host) {
-    observer_->OnTimingUpdated(render_frame_host, timing,
-                               mojom::PageLoadMetadata());
+    observer()->OnTimingUpdated(render_frame_host, timing,
+                                mojom::PageLoadMetadata());
   }
 
   void AttachObserver() {
-    embedder_interface_ =
-        new TestPageLoadMetricsEmbedderInterface(web_contents());
-    // Owned by the web_contents. Tests must be careful not to call
-    // SimulateTimingUpdate after they call DeleteContents() without also
-    // calling AttachObserver() again. Otherwise they will use-after-free the
-    // observer_.
-    observer_ = MetricsWebContentsObserver::CreateForWebContents(
-        web_contents(), base::WrapUnique(embedder_interface_));
-    observer_->WasShown();
+    embedder_interface_ = new TestPageLoadMetricsEmbedderInterface();

[Charlie Harrison, 2017/05/25 19:35:33]

optional nit to avoid bare new (I know this is old code)

auto embedder_interface = base::MakeUnique<TestInterface>();
embedder_interface_ = embedder_interface.get();

then std::move(embedder_interface) below

[Bryan McQuade, 2017/05/25 19:54:05]

Good idea, this makes it clearer that we're holding on to a unique_ptr. Done,
thanks!

+    MetricsWebContentsObserver* observer =
+        MetricsWebContentsObserver::CreateForWebContents(
+            web_contents(), base::WrapUnique(embedder_interface_));
+    observer->WasShown();
   }
 
   void CheckErrorEvent(InternalErrorLoadEvent error, int count) {
     histogram_tester_.ExpectBucketCount(internal::kErrorEvents, error, count);
     num_errors_ += count;
   }
 
   void CheckTotalErrorEvents() {
     histogram_tester_.ExpectTotalCount(internal::kErrorEvents, num_errors_);
   }
@@ skipping 29 matching lines @@
 
   const std::vector<GURL>& observed_committed_urls_from_on_start() const {
     return embedder_interface_->observed_committed_urls_from_on_start();
   }
 
   const std::vector<GURL>& completed_filtered_urls() const {
     return embedder_interface_->completed_filtered_urls();
   }
 
  protected:
+  MetricsWebContentsObserver* observer() {
+    return MetricsWebContentsObserver::FromWebContents(web_contents());
+  }
+
   base::HistogramTester histogram_tester_;
   TestPageLoadMetricsEmbedderInterface* embedder_interface_;
-  MetricsWebContentsObserver* observer_;
 
  private:
   int num_errors_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsWebContentsObserverTest);
 };
 
 TEST_F(MetricsWebContentsObserverTest, SuccessfulMainFrameNavigation) {
   mojom::PageLoadTiming timing;
   page_load_metrics::InitPageLoadTimingForTest(&timing);
@@ skipping 370 matching lines @@
 }
 
 TEST_F(MetricsWebContentsObserverTest, FlushMetricsOnAppEnterBackground) {
   content::WebContentsTester* web_contents_tester =
       content::WebContentsTester::For(web_contents());
   web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl));
 
   histogram_tester_.ExpectTotalCount(
       internal::kPageLoadCompletedAfterAppBackground, 0);
 
-  observer_->FlushMetricsOnAppEnterBackground();
+  observer()->FlushMetricsOnAppEnterBackground();
 
   histogram_tester_.ExpectTotalCount(
       internal::kPageLoadCompletedAfterAppBackground, 1);
   histogram_tester_.ExpectBucketCount(
       internal::kPageLoadCompletedAfterAppBackground, false, 1);
   histogram_tester_.ExpectBucketCount(
       internal::kPageLoadCompletedAfterAppBackground, true, 0);
 
   // Navigate again, which forces completion callbacks on the previous
   // navigation to be invoked.
@@ skipping 139 matching lines @@
   ASSERT_EQ(2, CountUpdatedTimingReported());
   ASSERT_EQ(0, CountEmptyCompleteTimingReported());
 
   ASSERT_EQ(1, CountUpdatedSubFrameTimingReported());
   EXPECT_TRUE(subframe_timing.Equals(*updated_subframe_timings().back()));
 
   CheckNoErrorEvents();
 }
 
 }  // namespace page_load_metrics