predictors: Call StopPrefetching with an url before redirect.

chrome/browser/predictors/resource_prefetch_predictor.cc

Index: chrome/browser/predictors/resource_prefetch_predictor.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc
index a2916bad115c7bec68f67b61b61c50f1a7bdc263..d7bb95b1b7ce485c18f220d10b45eb64cbecdfa5 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -606,6 +606,9 @@ void ResourcePrefetchPredictor::StartPrefetching(const GURL& url,
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ResourcePrefetcherManager::MaybeAddPrefetch,
                  prefetch_manager_, url, prediction.subresource_urls));
+
+  if (observer_)
+    observer_->OnPrefetchingStarted(url);
 }
 
 void ResourcePrefetchPredictor::StopPrefetching(const GURL& url) {
@@ -625,6 +628,9 @@ void ResourcePrefetchPredictor::StopPrefetching(const GURL& url) {
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ResourcePrefetcherManager::MaybeRemovePrefetch,
                  prefetch_manager_, url));
+
+  if (observer_)
+    observer_->OnPrefetchingStopped(url);
 }
 
 void ResourcePrefetchPredictor::OnPrefetchingFinished(
@@ -675,15 +681,22 @@ void ResourcePrefetchPredictor::OnMainFrameRequest(
 void ResourcePrefetchPredictor::OnMainFrameResponse(
     const URLRequestSummary& response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (initialization_state_ != INITIALIZED)

[alexilin, 2017/02/27 15:53:36]

This function shouldn't be called before initialization.
Aligned other On* functions as well.

-    return;
+  DCHECK_EQ(INITIALIZED, initialization_state_);
 
-  StopPrefetching(response.navigation_id.main_frame_url);
+  NavigationMap::iterator nav_it =
+      inflight_navigations_.find(response.navigation_id);
+  if (nav_it != inflight_navigations_.end()) {
+    // To match an URL in StartPrefetching().
+    StopPrefetching(nav_it->second->initial_url);
+  } else {
+    StopPrefetching(response.navigation_id.main_frame_url);

[alexilin, 2017/02/27 15:53:36]

I'm not sure if we need this fallback.

[Benoit L, 2017/02/27 17:31:19]

Why are you keeping it?

[alexilin, 2017/02/27 18:15:01]

Well, if for some reason we don't find an entry in inflight_navigations_ (we
should! but observers are tricky, some races are also possible) we still could
try to stop prefetching using main_frame_url. WDYT?

[Benoit L, 2017/02/28 14:26:30]

Acknowledged.

+  }
 }
 
 void ResourcePrefetchPredictor::OnMainFrameRedirect(
     const URLRequestSummary& response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(INITIALIZED, initialization_state_);
 
   const GURL& main_frame_url = response.navigation_id.main_frame_url;
   std::unique_ptr<PageRequestSummary> summary;
@@ -715,6 +728,7 @@ void ResourcePrefetchPredictor::OnMainFrameRedirect(
 void ResourcePrefetchPredictor::OnSubresourceResponse(
     const URLRequestSummary& response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(INITIALIZED, initialization_state_);
 
   NavigationMap::const_iterator nav_it =
         inflight_navigations_.find(response.navigation_id);
@@ -728,6 +742,7 @@ void ResourcePrefetchPredictor::OnSubresourceResponse(
 void ResourcePrefetchPredictor::OnNavigationComplete(
     const NavigationID& nav_id_without_timing_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(INITIALIZED, initialization_state_);
 
   NavigationMap::iterator nav_it =
       inflight_navigations_.find(nav_id_without_timing_info);
@@ -903,10 +918,15 @@ void ResourcePrefetchPredictor::CleanupAbandonedNavigations(
 
   for (auto it = inflight_prefetches_.begin();
        it != inflight_prefetches_.end();) {
-    if (time_now - it->second > max_navigation_age)
+    if (time_now - it->second > max_navigation_age) {
+      // It goes to the last bucket meaning that the duration was unlimited.
+      UMA_HISTOGRAM_TIMES(
+          internal::kResourcePrefetchPredictorPrefetchingDurationHistogram,
+          base::TimeTicks::Now() - it->second);

[Benoit L, 2017/02/27 17:31:19]

tiny nit: What computing the interval twice? Can you compute it before the if()
above, and use the value here as well?

[alexilin, 2017/02/27 18:15:01]

Done, thanks.
Actually, this value is always outside of histogram range thus we simply put it
into the last bucket.
For UMA_HISTOGRAM_TIMES max=10s and max_navigation_age=60s by default.

       it = inflight_prefetches_.erase(it);
-    else
+    } else {
       ++it;
+    }
   }
 
   // Remove old prefetches that haven't been claimed.