[page_load_metrics] Add a NavigationThrottle for richer abort metrics

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 260 matching lines @@
                       started_in_foreground_);
   }
 }
 
 PageLoadTracker::~PageLoadTracker() {
   const PageLoadExtraInfo info = ComputePageLoadExtraInfo();
 
   if (info.time_to_commit && renderer_tracked() && 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)
+
+  // Don't include any aborts that resulted in a new navigation, as the chain
+  // length will be included in the aborter PageLoadTracker.
+  if (commit_time_.is_null() && abort_type_ != ABORT_UNKNOWN_NAVIGATION &&
+      abort_type_ != ABORT_RELOAD && abort_type_ != ABORT_FORWARD_BACK &&
+      abort_type_ != ABORT_NEW_NAVIGATION) {
     LogAbortChainHistograms(nullptr);
+  }
 
   for (const auto& observer : observers_) {
     observer->OnComplete(timing_, info);
   }
 }
 
 void PageLoadTracker::LogAbortChainHistograms(
     content::NavigationHandle* final_navigation) {
   if (aborted_chain_size_ == 0)
     return;
@@ skipping 80 matching lines @@
   LogAbortChainHistograms(navigation_handle);
 }
 
 void PageLoadTracker::FailedProvisionalLoad(
     content::NavigationHandle* navigation_handle) {
   for (const auto& observer : observers_) {
     observer->OnFailedProvisionalLoad(navigation_handle);
   }
 }
 
+// TODO(csharrison): We can get an approximation of whether the abort is user
+// initiated or not from the NavigationHandle at this point.
+void PageLoadTracker::WillStartNavigationRequest(
+    content::NavigationHandle* navigation_handle) {
+  page_transition_ = navigation_handle->GetPageTransition();
+}
+
 void PageLoadTracker::Redirect(content::NavigationHandle* navigation_handle) {
   for (const auto& observer : observers_) {
     observer->OnRedirect(navigation_handle);
   }
 }
 
 void PageLoadTracker::OnInputEvent(const blink::WebInputEvent& event) {
   for (const auto& observer : observers_) {
     observer->OnUserInput(event);
   }
@@ skipping 128 matching lines @@
     return;
 
   UpdateAbortInternal(abort_type, timestamp, is_certainly_browser_timestamp);
 }
 
 void PageLoadTracker::UpdateAbort(UserAbortType abort_type,
                                   base::TimeTicks timestamp,
                                   bool is_certainly_browser_timestamp) {
   DCHECK_NE(abort_type, ABORT_NONE);
   DCHECK_NE(abort_type, ABORT_OTHER);
-  DCHECK_EQ(abort_type_, ABORT_OTHER);
+  DCHECK(abort_type_ == ABORT_OTHER || abort_type_ == ABORT_UNKNOWN_NAVIGATION)
+      << "UpdateAbort called with abort_type_ = " << abort_type_;
 
   // For some aborts (e.g. navigations), the initiated timestamp can be earlier
   // than the timestamp that aborted the load. Taking the minimum gives the
   // closest user initiated time known.
   UpdateAbortInternal(abort_type, std::min(abort_time_, timestamp),
                       is_certainly_browser_timestamp);
 }
 
-bool PageLoadTracker::IsLikelyProvisionalAbort(
-    base::TimeTicks abort_cause_time) {
+bool PageLoadTracker::WasAbortedRecently(base::TimeTicks abort_cause_time) {
   // Note that |abort_cause_time - abort_time| can be negative.
-  return abort_type_ == ABORT_OTHER &&
-         (abort_cause_time - abort_time_).InMilliseconds() < 100;
+  return (abort_cause_time - abort_time_).InMilliseconds() < 100;
 }
 
 bool PageLoadTracker::MatchesOriginalNavigation(
     content::NavigationHandle* navigation_handle) {
   // Neither navigation should have committed.
   DCHECK(!navigation_handle->HasCommitted());
   DCHECK(commit_time_.is_null());
   return navigation_handle->GetURL() == url_;
 }
 
@@ skipping 77 matching lines @@
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   bool handled = true;
   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;
 }
 
+// TODO(csharrison): The only reason DidStartNavigation is needed is for unit
+// tests. Otherwise all this logic can go in WillStartNavigationRequest.
 void MetricsWebContentsObserver::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame())
     return;
   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 (!aborted_provisional_loads_.empty()) {
+    PageLoadTracker* last_aborted = aborted_provisional_loads_.back().get();
     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) {
       LogAbortChainSameURLHistogram(
           last_aborted->aborted_chain_size_same_url());
     }
     chain_size = last_aborted->aborted_chain_size() + 1;
   }
 
   // Pass in the last committed url to the PageLoadTracker. If the MWCO has
@@ skipping 11 matching lines @@
   has_navigated_ = true;
 
   // We can have two provisional loads in some cases. E.g. a same-site
   // navigation can have a concurrent cross-process navigation started
   // from the omnibox.
   DCHECK_GT(2ul, provisional_loads_.size());
   // Passing raw pointers to observers_ and embedder_interface_ is safe because
   // the MetricsWebContentsObserver owns them both list and they are torn down
   // after the PageLoadTracker. The PageLoadTracker does not hold on to
   // committed_load_ or navigation_handle beyond the scope of the constructor.
-  provisional_loads_.insert(std::make_pair(
+  auto it = provisional_loads_.insert(std::make_pair(
       navigation_handle,
       base::WrapUnique(new PageLoadTracker(
           in_foreground_, embedder_interface_.get(), currently_committed_url,
           navigation_handle, chain_size, chain_size_same_url))));
+  // Don't clear the aborted provisional loads here because
+  // WillStartNavigationRequest will soon be called, providing a better value
+  // for the page transition and user gesture.
+  NotifyAbortedProvisionalLoadsNewNavigation(it.first->second.get());

[Bryan McQuade, 2016/07/13 18:23:36]

should we not call this and just wait to invoke it in WillStartNavRequest? What
is the benefit to calling it here as well?

[Charlie Harrison, 2016/07/13 18:58:19]

It is only really for current unit tests which do not call WillStartNavRequest
:(
I am not sure all the are cases where a load can be aborted between start and
WillStartNavRequest, but this code also guards against that.

+}
+
+void MetricsWebContentsObserver::WillStartNavigationRequest(
+    content::NavigationHandle* navigation_handle) {
+  auto it = provisional_loads_.find(navigation_handle);
+  if (it == provisional_loads_.end())
+    return;
+  it->second->WillStartNavigationRequest(navigation_handle);
+  NotifyAbortedProvisionalLoadsNewNavigation(it->second.get());
+  aborted_provisional_loads_.clear();
 }
 
 const PageLoadExtraInfo
 MetricsWebContentsObserver::GetPageLoadExtraInfoForCommittedLoad() {
   DCHECK(committed_load_);
   return committed_load_->ComputePageLoadExtraInfo();
 }
 
 void MetricsWebContentsObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
@@ skipping 140 matching lines @@
 }
 
 void MetricsWebContentsObserver::NotifyAbortAllLoads(UserAbortType abort_type) {
   NotifyAbortAllLoadsWithTimestamp(abort_type, base::TimeTicks::Now(), true);
 }
 
 void MetricsWebContentsObserver::NotifyAbortAllLoadsWithTimestamp(
     UserAbortType abort_type,
     base::TimeTicks timestamp,
     bool is_certainly_browser_timestamp) {
-  if (committed_load_)
+  if (committed_load_) {
     committed_load_->NotifyAbort(abort_type, timestamp,
+
                                  is_certainly_browser_timestamp);
+  }
   for (const auto& kv : provisional_loads_) {
     kv.second->NotifyAbort(abort_type, timestamp,
                            is_certainly_browser_timestamp);
   }
   for (const auto& tracker : aborted_provisional_loads_) {
-    if (tracker->IsLikelyProvisionalAbort(timestamp))
+    // Only update an aborted provisional load if it has an OTHER type. Do not
+    // include UNKNOWN_NAVIGATION here because those are handled in
+    // NotifyAbortedProvisionalLoadsNewNavigation.
+    // TODO(csharrison): This is an implementation flaw due to how the unit

[Bryan McQuade, 2016/07/13 18:23:36]

which tests in particular break due to this? is it just the MWCO unit test? Or
do our observer tests also break? How much work would it take to fix the test
limitation?

[Charlie Harrison, 2016/07/13 18:58:19]

The tests that fail if we include UNKNOWN_NAVIGATION here are:
AbortsPageLoadMetricsObserverTest.UnknownNavigationBeforeCommit
FromGWSPageLoadMetricsObserverTest.UnknownNavigationBeforeCommit
MetricsWebContentsObserverTest.LogAbortChainsNoCommit

Due to another abort reason occurring to a load that was aborted with
UNKNOWN_NAVIGATION.

I looked at the test infra and it seemed relatively daunting to fix this at the
fundamental level. clamy@, do you have an idea of how difficult it would be to
fully integrate NavigationThrottle/Handles in unit tests?

+    // testing framework avoids calling WillStartNavigationRequest.
+    if (tracker->WasAbortedRecently(timestamp) &&
+        tracker->abort_type() == ABORT_OTHER) {
       tracker->UpdateAbort(abort_type, timestamp,
                            is_certainly_browser_timestamp);
+    }
   }
   aborted_provisional_loads_.clear();
 }
 
-std::unique_ptr<PageLoadTracker>
-MetricsWebContentsObserver::NotifyAbortedProvisionalLoadsNewNavigation(
-    content::NavigationHandle* new_navigation) {
-  // If there are multiple aborted loads that can be attributed to this one,
-  // just count the latest one for simplicity. Other loads will fall into the
-  // OTHER bucket, though there shouldn't be very many.
+void MetricsWebContentsObserver::NotifyAbortedProvisionalLoadsNewNavigation(
+    PageLoadTracker* new_navigation) {

[Bryan McQuade, 2016/07/13 18:23:36]

nit: given that the type is now a PageLoadTracker let's rename to new_page_load.
Similarly, let's rename the method NotifyAbortedProvisionalLoadsNewPageLoad

[Charlie Harrison, 2016/07/13 18:58:19]

Done.

   if (aborted_provisional_loads_.size() == 0)
-    return nullptr;
+    return;
   if (aborted_provisional_loads_.size() > 1)
     RecordInternalError(ERR_NAVIGATION_SIGNALS_MULIPLE_ABORTED_LOADS);
 
-  std::unique_ptr<PageLoadTracker> last_aborted_load =
-      std::move(aborted_provisional_loads_.back());
-  aborted_provisional_loads_.pop_back();
-
-  base::TimeTicks timestamp = new_navigation->NavigationStart();
-  if (last_aborted_load->IsLikelyProvisionalAbort(timestamp))
-    last_aborted_load->UpdateAbort(ABORT_UNKNOWN_NAVIGATION, timestamp, false);
-
-  aborted_provisional_loads_.clear();
-  return last_aborted_load;
+  for (const auto& it : aborted_provisional_loads_) {
+    base::TimeTicks timestamp = new_navigation->navigation_start();
+    if (it->WasAbortedRecently(timestamp) &&
+        (it->abort_type() == ABORT_OTHER ||
+         it->abort_type() == ABORT_UNKNOWN_NAVIGATION)) {
+      const base::Optional<ui::PageTransition>& transition =
+          new_navigation->page_transition();
+      UserAbortType abort_type =
+          transition ? AbortTypeForPageTransition(transition.value())
+                     : ABORT_UNKNOWN_NAVIGATION;
+      it->UpdateAbort(abort_type, timestamp, false);
+    }
+  }
 }
 
 void MetricsWebContentsObserver::OnTimingUpdated(
     content::RenderFrameHost* render_frame_host,
     const PageLoadTiming& timing,
     const PageLoadMetadata& metadata) {
   bool error = false;
   if (!committed_load_ || !committed_load_->renderer_tracked()) {
     RecordInternalError(ERR_IPC_WITH_NO_RELEVANT_LOAD);
     error = true;
@@ skipping 19 matching lines @@
 
   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);
   }
 }
 
 }  // namespace page_load_metrics