predictors: Call StopPrefetching with an url before redirect.

chrome/browser/predictors/resource_prefetch_predictor.cc

 // Copyright 2014 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/predictors/resource_prefetch_predictor.h"
 
 #include <map>
 #include <set>
 #include <utility>
 
@@ skipping 588 matching lines @@
     return;
 
   ResourcePrefetchPredictor::Prediction prediction;
   if (!GetPrefetchData(url, &prediction))
     return;
 
   BrowserThread::PostTask(
       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) {
   TRACE_EVENT1("browser", "ResourcePrefetchPredictor::StopPrefetching", "url",
                url.spec());
   auto it = inflight_prefetches_.find(url);
   if (it != inflight_prefetches_.end()) {
     UMA_HISTOGRAM_TIMES(
         internal::kResourcePrefetchPredictorPrefetchingDurationHistogram,
         base::TimeTicks::Now() - it->second);
     inflight_prefetches_.erase(it);
   }
   if (!prefetch_manager_.get())  // Not enabled.
     return;
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ResourcePrefetcherManager::MaybeRemovePrefetch,
                  prefetch_manager_, url));
+
+  if (observer_)
+    observer_->OnPrefetchingStopped(url);
 }
 
 void ResourcePrefetchPredictor::OnPrefetchingFinished(
     const GURL& main_frame_url,
     std::unique_ptr<ResourcePrefetcher::PrefetcherStats> stats) {
   if (observer_)
     observer_->OnPrefetchingFinished(main_frame_url);
 
   prefetcher_stats_.insert(std::make_pair(main_frame_url, std::move(stats)));
 }
@@ skipping 30 matching lines @@
 
   // New empty navigation entry.
   inflight_navigations_.insert(
       std::make_pair(request.navigation_id,
                      base::MakeUnique<PageRequestSummary>(main_frame_url)));
 }
 
 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;
   NavigationMap::iterator nav_it =
       inflight_navigations_.find(response.navigation_id);
   if (nav_it != inflight_navigations_.end()) {
     summary.reset(nav_it->second.release());
     inflight_navigations_.erase(nav_it);
   }
 
@@ skipping 11 matching lines @@
   NavigationID navigation_id(response.navigation_id);
   navigation_id.main_frame_url = response.redirect_url;
   summary->main_frame_url = response.redirect_url;
   inflight_navigations_.insert(
       std::make_pair(navigation_id, std::move(summary)));
 }
 
 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);
   if (nav_it == inflight_navigations_.end()) {
     return;
   }
 
   nav_it->second->subresource_requests.push_back(response);
 }
 
 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);
   if (nav_it == inflight_navigations_.end())
     return;
 
   // Remove the navigation from the inflight navigations.
   std::unique_ptr<PageRequestSummary> summary = std::move(nav_it->second);
   inflight_navigations_.erase(nav_it);
 
@@ skipping 155 matching lines @@
     if ((it->first.tab_id == navigation_id.tab_id) ||
         (time_now - it->first.creation_time > max_navigation_age)) {
       inflight_navigations_.erase(it++);
     } else {
       ++it;
     }
   }
 
   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.
   for (auto stats_it = prefetcher_stats_.begin();
        stats_it != prefetcher_stats_.end();) {
     if (time_now - stats_it->second->start_time > max_navigation_age) {
       // No requests -> everything is a miss.
       ReportPrefetchAccuracy(*stats_it->second,
                              std::vector<URLRequestSummary>());
       stats_it = prefetcher_stats_.erase(stats_it);
@@ skipping 473 matching lines @@
 TestObserver::~TestObserver() {
   predictor_->SetObserverForTesting(nullptr);
 }
 
 TestObserver::TestObserver(ResourcePrefetchPredictor* predictor)
     : predictor_(predictor) {
   predictor_->SetObserverForTesting(this);
 }
 
 }  // namespace predictors