Standardize which page loads are tracked

components/page_load_metrics/browser/metrics_web_contents_observer.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 "components/page_load_metrics/browser/metrics_web_contents_observer.h"
 
 #include <algorithm>
 #include <ostream>
 #include <string>
 #include <utility>
@@ skipping 46 matching lines @@
 // Helper to allow use of Optional<> values in LOG() messages.
 std::ostream& operator<<(std::ostream& os,
                          const base::Optional<base::TimeDelta>& opt) {
   if (opt)
     os << opt.value();
   else
     os << "(unset)";
   return os;
 }
 
-// The url we see from the renderer side is not always the same as what
-// we see from the browser side (e.g. chrome://newtab). We want to be
-// sure here that we aren't logging UMA for internal pages.
-bool IsRelevantNavigation(content::NavigationHandle* navigation_handle,
-                          const GURL& browser_url,
-                          const std::string& mime_type) {
-  DCHECK(navigation_handle->HasCommitted());
-  return navigation_handle->IsInMainFrame() &&
-         !navigation_handle->IsSamePage() &&
-         !navigation_handle->IsErrorPage() &&
-         navigation_handle->GetURL().SchemeIsHTTPOrHTTPS() &&
-         browser_url.SchemeIsHTTPOrHTTPS() &&
-         (mime_type == "text/html" || mime_type == "application/xhtml+xml");
-}
-
 // If second is non-zero, first must also be non-zero and less than or equal to
 // second.
 bool EventsInOrder(const base::Optional<base::TimeDelta>& first,
                    const base::Optional<base::TimeDelta>& second) {
   if (!second) {
     return true;
   }
   return first && first <= second;
 }
 
@@ skipping 124 matching lines @@
     UMA_HISTOGRAM_COUNTS(internal::kAbortChainSizeSameURL,
                          aborted_chain_size_same_url);
   }
 }
 
 void DispatchObserverTimingCallbacks(PageLoadMetricsObserver* observer,
                                      const PageLoadTiming& last_timing,
                                      const PageLoadTiming& new_timing,
                                      const PageLoadMetadata& last_metadata,
                                      const PageLoadExtraInfo& extra_info) {
-  observer->OnTimingUpdate(new_timing, extra_info);
+  if (last_timing != new_timing)
+    observer->OnTimingUpdate(new_timing, extra_info);
   if (new_timing.dom_content_loaded_event_start &&
       !last_timing.dom_content_loaded_event_start)
     observer->OnDomContentLoadedEventStart(new_timing, extra_info);
   if (new_timing.load_event_start && !last_timing.load_event_start)
     observer->OnLoadEventStart(new_timing, extra_info);
   if (new_timing.first_layout && !last_timing.first_layout)
     observer->OnFirstLayout(new_timing, extra_info);
   if (new_timing.first_paint && !last_timing.first_paint)
     observer->OnFirstPaint(new_timing, extra_info);
   if (new_timing.first_text_paint && !last_timing.first_text_paint)
@@ skipping 12 matching lines @@
 
 }  // namespace
 
 PageLoadTracker::PageLoadTracker(
     bool in_foreground,
     PageLoadMetricsEmbedderInterface* embedder_interface,
     const GURL& currently_committed_url,
     content::NavigationHandle* navigation_handle,
     int aborted_chain_size,
     int aborted_chain_size_same_url)
-    : renderer_tracked_(false),
+    : did_stop_tracking_(false),
       navigation_start_(navigation_handle->NavigationStart()),
       url_(navigation_handle->GetURL()),
       abort_type_(ABORT_NONE),
       started_in_foreground_(in_foreground),
       aborted_chain_size_(aborted_chain_size),
       aborted_chain_size_same_url_(aborted_chain_size_same_url),
       embedder_interface_(embedder_interface) {
   DCHECK(!navigation_handle->HasCommitted());
   embedder_interface_->RegisterObservers(this);
   for (const auto& observer : observers_) {
     observer->OnStart(navigation_handle, currently_committed_url,
                       started_in_foreground_);
   }
 }
 
 PageLoadTracker::~PageLoadTracker() {
+  if (did_stop_tracking_)
+    return;
+
   const PageLoadExtraInfo info = ComputePageLoadExtraInfo();
 
-  if (info.time_to_commit && renderer_tracked() && timing_.IsEmpty()) {
+  if (info.time_to_commit && timing_.IsEmpty()) {
     RecordInternalError(ERR_NO_IPCS_RECEIVED);
   }
   // Recall that trackers that are given ABORT_UNKNOWN_NAVIGATION have their
   // chain length added to the next navigation. Take care not to double count
   // them. Also do not double count committed loads, which call this already.
   if (commit_time_.is_null() && abort_type_ != ABORT_UNKNOWN_NAVIGATION)
     LogAbortChainHistograms(nullptr);
 
   for (const auto& observer : observers_) {
     observer->OnComplete(timing_, info);
@@ skipping 149 matching lines @@
     const PageLoadExtraInfo info = ComputePageLoadExtraInfo();
     for (const auto& observer : observers_) {
       DispatchObserverTimingCallbacks(observer.get(), last_timing, new_timing,
                                       last_metadata, info);
     }
     return true;
   }
   return false;
 }
 
-void PageLoadTracker::set_renderer_tracked(bool renderer_tracked) {
-  renderer_tracked_ = renderer_tracked;
+void PageLoadTracker::StopTracking() {
+  did_stop_tracking_ = true;
 }
 
 void PageLoadTracker::AddObserver(
     std::unique_ptr<PageLoadMetricsObserver> observer) {
   observers_.push_back(std::move(observer));
 }
 
 void PageLoadTracker::ClampBrowserTimestampIfInterProcessTimeTickSkew(
     base::TimeTicks* event_time) {
   DCHECK(event_time != nullptr);
   // Windows 10 GCE bot non-deterministically failed because TimeTicks::Now()
   // called in the browser process e.g. commit_time was less than
   // navigation_start_ that was populated in the renderer process because the
   // clock was not system-wide monotonic.
   // Note that navigation_start_ can also be set in the browser process in
   // some cases and in those cases event_time should never be <
   // navigation_start_. If it is due to a code error and it gets clamped in this
   // function, on high resolution systems it should lead to a dcheck failure.
 
-  // TODO (shivanisha) Currently IsHighResolution is the best way to check
+  // TODO(shivanisha): Currently IsHighResolution is the best way to check
   // if the clock is system-wide monotonic. However IsHighResolution
   // does a broader check to see if the clock in use is high resolution
   // which also implies it is system-wide monotonic (on Windows).
   if (base::TimeTicks::IsHighResolution()) {
     DCHECK(event_time->is_null() || *event_time >= navigation_start_);
     return;
   }
 
   if (!event_time->is_null() && *event_time < navigation_start_) {
     RecordInternalError(ERR_INTER_PROCESS_TIME_TICK_SKEW);
@@ skipping 157 matching lines @@
   IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(MetricsWebContentsObserver, message,
                                    render_frame_host)
     IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_TimingUpdated, OnTimingUpdated)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void MetricsWebContentsObserver::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (!navigation_handle->IsInMainFrame())
+  if (!navigation_handle->IsInMainFrame() ||
+      !ShouldTrackNavigation(navigation_handle))
     return;
+
+  // TODO(bmcquade): add support for tracking prerendered pages when they become
+  // visible.
   if (embedder_interface_->IsPrerendering(web_contents()))
     return;
-  if (embedder_interface_->IsNewTabPageUrl(navigation_handle->GetURL()))
-    return;
-  if (navigation_handle->GetURL().spec().compare(url::kAboutBlankURL) == 0)
-    return;
 
   std::unique_ptr<PageLoadTracker> last_aborted =
       NotifyAbortedProvisionalLoadsNewNavigation(navigation_handle);
 
   int chain_size_same_url = 0;
   int chain_size = 0;
   if (last_aborted) {
     if (last_aborted->MatchesOriginalNavigation(navigation_handle)) {
       chain_size_same_url = last_aborted->aborted_chain_size_same_url() + 1;
     } else if (last_aborted->aborted_chain_size_same_url() > 0) {
@@ skipping 40 matching lines @@
 
 void MetricsWebContentsObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame())
     return;
 
   std::unique_ptr<PageLoadTracker> finished_nav(
       std::move(provisional_loads_[navigation_handle]));
   provisional_loads_.erase(navigation_handle);
 
-  // There's a chance a navigation could have started before we were added to a
-  // tab. Bail out early if this is the case.
-  if (!finished_nav)
-    return;
+  const bool should_track =
+      finished_nav && ShouldTrackNavigation(navigation_handle);
 
-  // Handle a pre-commit error here. Navigations that result in an error page
-  // will be ignored. Note that downloads/204s will result in HasCommitted()
-  // returning false.
-  // TODO(csharrison): Track changes to NavigationHandle for signals when this
-  // is the case (HTTP response headers).
-  if (!navigation_handle->HasCommitted()) {
-    finished_nav->FailedProvisionalLoad(navigation_handle);
+  if (finished_nav && !should_track)
+    finished_nav->StopTracking();
 
-    net::Error error = navigation_handle->GetNetErrorCode();
+  if (navigation_handle->HasCommitted()) {
+    // Ignore same-page navigations.
+    if (navigation_handle->IsSamePage())
+      return;
 
-    // net::OK: This case occurs when the NavigationHandle finishes and reports
-    // !HasCommitted(), but reports no net::Error. This should not occur
-    // pre-PlzNavigate, but afterwards it should represent the navigation
-    // stopped by the user before it was ready to commit.
-    // net::ERR_ABORTED: An aborted provisional load has error net::ERR_ABORTED.
-    // Note that this can come from some non user-initiated errors, such as
-    // downloads, or 204 responses. See crbug.com/542369.
-    if ((error == net::OK) || (error == net::ERR_ABORTED)) {
-      finished_nav->NotifyAbort(ABORT_OTHER, base::TimeTicks::Now(), true);
-      aborted_provisional_loads_.push_back(std::move(finished_nav));
+    if (should_track) {
+      HandleCommittedNavigationForTrackedLoad(navigation_handle,
+                                              std::move(finished_nav));
+    } else {
+      committed_load_.reset();
     }
+  } else if (should_track) {
+    HandleFailedNavigationForTrackedLoad(navigation_handle,
+                                         std::move(finished_nav));
+  }
+}
 
-    return;
+// Handle a pre-commit error. Navigations that result in an error page will be
+// ignored. Note that downloads/204s will result in HasCommitted() returning
+// false.
+// TODO(csharrison): Track changes to NavigationHandle for signals when this is
+// the case (HTTP response headers).
+void MetricsWebContentsObserver::HandleFailedNavigationForTrackedLoad(
+    content::NavigationHandle* navigation_handle,
+    std::unique_ptr<PageLoadTracker> tracker) {
+  tracker->FailedProvisionalLoad(navigation_handle);
+
+  net::Error error = navigation_handle->GetNetErrorCode();
+
+  // net::OK: This case occurs when the NavigationHandle finishes and reports
+  // !HasCommitted(), but reports no net::Error. This should not occur
+  // pre-PlzNavigate, but afterwards it should represent the navigation stopped
+  // by the user before it was ready to commit.
+  // net::ERR_ABORTED: An aborted provisional load has error
+  // net::ERR_ABORTED. Note that this can come from some non user-initiated
+  // errors, such as downloads, or 204 responses. See crbug.com/542369.
+  if ((error == net::OK) || (error == net::ERR_ABORTED)) {
+    tracker->NotifyAbort(ABORT_OTHER, base::TimeTicks::Now(), true);
+    aborted_provisional_loads_.push_back(std::move(tracker));
   }
+}
 
-  // Don't treat a same-page nav as a new page load.
-  if (navigation_handle->IsSamePage())
-    return;
-
+void MetricsWebContentsObserver::HandleCommittedNavigationForTrackedLoad(
+    content::NavigationHandle* navigation_handle,
+    std::unique_ptr<PageLoadTracker> tracker) {
   if (!navigation_handle->HasUserGesture() &&
       (navigation_handle->GetPageTransition() &
        ui::PAGE_TRANSITION_CLIENT_REDIRECT) != 0 &&
-      committed_load_)
-    committed_load_->NotifyClientRedirectTo(*finished_nav);
+      committed_load_ && tracker)

[Charlie Harrison, 2016/07/15 13:01:17]

Can we DCHECK(tracker) here instead if keeping it in the if statement?

[Bryan McQuade, 2016/07/15 14:14:42]

good catch, thanks! this test was left over from an earlier version of the
refactor and is no longer necessary, so i've removed it w/o a dcheck - the
expectation is that this method should never receive a null tracker (other parts
of the function depend on this being the case already).

[Charlie Harrison, 2016/07/15 14:19:55]

Fair enough, though a DCHECK just solidifies this assumption. No big deal either
way.

+    committed_load_->NotifyClientRedirectTo(*tracker);
 
   // Notify other loads that they may have been aborted by this committed load.
   // Note that by using the committed navigation start as the abort cause, we
   // lose data on provisional loads that were aborted by other provisional
   // loads. Those will either be listed as ABORT_OTHER or as being aborted by
   // this load.
   // is_certainly_browser_timestamp is set to false because NavigationStart()
   // could be set in either the renderer or browser process.
   NotifyAbortAllLoadsWithTimestamp(
       AbortTypeForPageTransition(navigation_handle->GetPageTransition()),

[Bryan McQuade, 2016/07/15 14:14:42]

looking at this bit of code, should we be notifying other loads that they
might've been aborted only for tracked commits, or should we be doing so for
untracked commit as well? for instance, if I have a provisional nav go
http://www.google.com/ and that gets aborted by a nav to a chrome:// navigation
that commits, it seems like we may want to notify the google.com load that it
was aborted (due to the chrome nav's commit), even though the chrome nav isn't
tracked. what do you think?

[Charlie Harrison, 2016/07/15 14:19:55]

Oh good catch. Can you actually put this code in the caller, after the check for
an in-page navigation?

We want this to be called for any committed load.

       navigation_handle->NavigationStart(), false);
 
-  committed_load_ = std::move(finished_nav);
-  aborted_provisional_loads_.clear();
-
-  const GURL& browser_url = web_contents()->GetLastCommittedURL();
-  const std::string& mime_type = web_contents()->GetContentsMimeType();
-  DCHECK(!browser_url.is_empty());
-  DCHECK(!mime_type.empty());
-  committed_load_->set_renderer_tracked(
-      IsRelevantNavigation(navigation_handle, browser_url, mime_type));
-
+  committed_load_ = std::move(tracker);
   committed_load_->Commit(navigation_handle);
 }
 
 void MetricsWebContentsObserver::NavigationStopped() {
   NotifyAbortAllLoads(ABORT_STOP);
 }
 
 void MetricsWebContentsObserver::OnInputEvent(
     const blink::WebInputEvent& event) {
   // Ignore browser navigation or reload which comes with type Undefined.
@@ skipping 101 matching lines @@
 
   aborted_provisional_loads_.clear();
   return last_aborted_load;
 }
 
 void MetricsWebContentsObserver::OnTimingUpdated(
     content::RenderFrameHost* render_frame_host,
     const PageLoadTiming& timing,
     const PageLoadMetadata& metadata) {
   bool error = false;
-  if (!committed_load_ || !committed_load_->renderer_tracked()) {
+  if (!committed_load_) {
     RecordInternalError(ERR_IPC_WITH_NO_RELEVANT_LOAD);
     error = true;
   }
 
   // We may receive notifications from frames that have been navigated away
   // from. We simply ignore them.
   if (render_frame_host != web_contents()->GetMainFrame()) {
     RecordInternalError(ERR_IPC_FROM_WRONG_FRAME);
     error = true;
   }
@@ skipping 10 matching lines @@
     return;
 
   if (!committed_load_->UpdateTiming(timing, metadata)) {
     // If the page load tracker cannot update its timing, something is wrong
     // with the IPC (it's from another load, or it's invalid in some other way).
     // We expect this to be a rare occurrence.
     RecordInternalError(ERR_BAD_TIMING_IPC);
   }
 }
 
+bool MetricsWebContentsObserver::ShouldTrackNavigation(
+    content::NavigationHandle* navigation_handle) const {
+  DCHECK(navigation_handle->IsInMainFrame());
+  if (!navigation_handle->GetURL().SchemeIsHTTPOrHTTPS())
+    return false;
+  if (embedder_interface_->IsNewTabPageUrl(navigation_handle->GetURL()))
+    return false;
+  if (navigation_handle->HasCommitted()) {
+    if (navigation_handle->IsSamePage() || navigation_handle->IsErrorPage())
+      return false;
+    const std::string& mime_type = web_contents()->GetContentsMimeType();
+    if (mime_type != "text/html" && mime_type != "application/xhtml+xml")
+      return false;
+  }
+  return true;
+}
+
 }  // namespace page_load_metrics