predictors: move ResourcePrefetcher handling to LoadingPredictor.

chrome/browser/predictors/loading_predictor.cc

 // Copyright 2017 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/loading_predictor.h"
 
+#include <vector>
+
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "chrome/browser/predictors/loading_data_collector.h"
 #include "chrome/browser/predictors/loading_stats_collector.h"
 #include "chrome/browser/predictors/resource_prefetch_common.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 
 namespace predictors {
 
 using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
 
 LoadingPredictor::LoadingPredictor(const LoadingPredictorConfig& config,
                                    Profile* profile)
     : config_(config),
       profile_(profile),
       resource_prefetch_predictor_(
           base::MakeUnique<ResourcePrefetchPredictor>(config, profile)),
       stats_collector_(base::MakeUnique<LoadingStatsCollector>(
           resource_prefetch_predictor_.get(),
           config)),
       loading_data_collector_(base::MakeUnique<LoadingDataCollector>(
           resource_prefetch_predictor())),
+      observer_(nullptr),
       weak_factory_(this) {
   resource_prefetch_predictor_->SetStatsCollector(stats_collector_.get());
 }
 
 LoadingPredictor::~LoadingPredictor() = default;
 
 void LoadingPredictor::PrepareForPageLoad(const GURL& url, HintOrigin origin) {
   if (active_hints_.find(url) != active_hints_.end())
     return;
   ResourcePrefetchPredictor::Prediction prediction;
   if (!resource_prefetch_predictor_->GetPrefetchData(url, &prediction))
     return;
 
   // To report hint durations.
   active_hints_.emplace(url, base::TimeTicks::Now());
-
-  if (config_.IsPrefetchingEnabledForOrigin(profile_, origin))
-    resource_prefetch_predictor_->StartPrefetching(url, prediction);
+  MaybeAddPrefetch(url, prediction, origin);
 }
 
 void LoadingPredictor::CancelPageLoadHint(const GURL& url) {
   CancelActiveHint(active_hints_.find(url));
 }
 
 void LoadingPredictor::StartInitialization() {
   resource_prefetch_predictor_->StartInitialization();
 }
 
 LoadingDataCollector* LoadingPredictor::loading_data_collector() {
   return loading_data_collector_.get();
 }
 
 ResourcePrefetchPredictor* LoadingPredictor::resource_prefetch_predictor() {
   return resource_prefetch_predictor_.get();
 }
 
 void LoadingPredictor::Shutdown() {

[alexilin, 2017/07/03 14:55:01]

Could LoadingPredictor be destructed without a call to Shutdown()? Probably
it'll mean something bad has already happened anyway, so it's okay to destruct
prefetches on UI thread.

[Benoit L, 2017/07/03 15:43:14]

No, it can't.
Unless we crashed, in which case no destructor is called.

   resource_prefetch_predictor_->Shutdown();
+
+  std::vector<std::unique_ptr<ResourcePrefetcher>> prefetchers;
+  for (auto& kv : prefetches_)
+    prefetchers.push_back(std::move(kv.second.first));
+
+  // prefetchers must be destroyed on the IO thread.

[alexilin, 2017/07/03 14:55:01]

nit:
s/prefetchers/|prefetchers|/

[Benoit L, 2017/07/03 15:43:14]

Done.

+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce(
+          [](std::vector<std::unique_ptr<ResourcePrefetcher>> prefetchers) {},
+          std::move(prefetchers)));
 }
 
 void LoadingPredictor::OnMainFrameRequest(const URLRequestSummary& summary) {
   DCHECK(summary.resource_type == content::RESOURCE_TYPE_MAIN_FRAME);
 
   const NavigationID& navigation_id = summary.navigation_id;
   CleanupAbandonedHintsAndNavigations(navigation_id);
   active_navigations_.emplace(navigation_id, navigation_id.main_frame_url);
   PrepareForPageLoad(navigation_id.main_frame_url, HintOrigin::NAVIGATION);
 }
@@ skipping 19 matching lines @@
   auto it = active_navigations_.find(navigation_id);
   if (it != active_navigations_.end()) {
     const GURL& initial_url = it->second;
     CancelPageLoadHint(initial_url);
     active_navigations_.erase(it);
   } else {
     CancelPageLoadHint(navigation_id.main_frame_url);
   }
 }
 
+void LoadingPredictor::SetObserverForTesting(TestLoadingObserver* observer) {
+  observer_ = observer;
+}
+
 std::map<GURL, base::TimeTicks>::iterator LoadingPredictor::CancelActiveHint(
     std::map<GURL, base::TimeTicks>::iterator hint_it) {
   if (hint_it == active_hints_.end())
     return hint_it;
 
   const GURL& url = hint_it->first;
-  resource_prefetch_predictor_->StopPrefetching(url);
+  MaybeRemovePrefetch(url);
 
   UMA_HISTOGRAM_TIMES(
       internal::kResourcePrefetchPredictorPrefetchingDurationHistogram,
       base::TimeTicks::Now() - hint_it->second);
   return active_hints_.erase(hint_it);
 }
 
 void LoadingPredictor::CleanupAbandonedHintsAndNavigations(
     const NavigationID& navigation_id) {
   base::TimeTicks time_now = base::TimeTicks::Now();
@@ skipping 19 matching lines @@
         (time_now - it->first.creation_time > max_navigation_age)) {
       const GURL& initial_url = it->second;
       CancelActiveHint(active_hints_.find(initial_url));
       it = active_navigations_.erase(it);
     } else {
       ++it;
     }
   }
 }
 
+void LoadingPredictor::MaybeAddPrefetch(
+    const GURL& url,
+    const ResourcePrefetchPredictor::Prediction& prediction,
+    HintOrigin origin) {
+  if (!config_.IsPrefetchingEnabledForOrigin(profile_, origin))
+    return;
+  std::string host = url.host();
+  if (prefetches_.find(host) != prefetches_.end())
+    return;
+
+  auto prefetcher = base::MakeUnique<ResourcePrefetcher>(
+      GetWeakPtr(), profile_->GetRequestContext(),
+      config_.max_prefetches_inflight_per_navigation,
+      config_.max_prefetches_inflight_per_host_per_navigation, url,
+      prediction.subresource_urls);

[alexilin, 2017/07/03 14:55:01]

std::move(prediction.subresource_urls)
|prediction| can be moved in this function as well.

[Benoit L, 2017/07/03 15:43:14]

Thanks!

Done.

[alexilin, 2017/07/03 15:57:14]

I'm very suspicious about moving an object (or its part) that was passed by a
const reference. Could you rewrite the code to avoid that? Probably, it would be
better to pass |prediction| here as
unique_ptr<ResourcePrefetchPredictor::Prediction>

+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&ResourcePrefetcher::Start,
+                     base::Unretained(prefetcher.get())));

[alexilin, 2017/07/03 14:55:01]

Can you add a comment explaining that passing unretained pointer is okay in this
case because a prefetcher destruction will be scheduled on the IO thread after
that always?

[Benoit L, 2017/07/03 15:43:14]

Done.

+  prefetches_.emplace(host, std::make_pair(std::move(prefetcher), false));
+  if (observer_)
+    observer_->OnPrefetchingStarted(url);
+}
+
+void LoadingPredictor::MaybeRemovePrefetch(const GURL& url) {
+  std::string host = url.host();
+  auto it = prefetches_.find(host);
+  if (it == prefetches_.end())
+    return;
+
+  auto& prefetcher_and_deleted = it->second;
+  if (prefetcher_and_deleted.second)
+    return;
+
+  // Avoid duplicates.

[alexilin, 2017/07/03 14:55:01]

Can you make this comment more detailed? Something like
"Avoid to stop the same prefetcher twice."

[Benoit L, 2017/07/03 15:43:14]

Done.

+  prefetcher_and_deleted.second = true;
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&ResourcePrefetcher::Stop,
+                     base::Unretained(prefetcher_and_deleted.first.get())));
+  if (observer_)
+    observer_->OnPrefetchingStopped(url);
+}
+
+void LoadingPredictor::ResourcePrefetcherFinished(
+    ResourcePrefetcher* prefetcher,

[alexilin, 2017/07/03 14:55:01]

nit:
Do we really need to pass a pointer to ResourcePrefetcher? It seems unnecessary
and makes me more worried about passing raw ptrs between threads.
PrefetcherStats have a |url| field. On the other hand it could be not right to
wire business logic to stats-reporting logic.

[Benoit L, 2017/07/03 15:43:14]

The lifetime of a ResourcePrefetcher is entirely controlled by this object, that
is LoadingPredictor owns the ResourcePrefetcher, so I think that passing
pointers is fine here.

+    std::unique_ptr<ResourcePrefetcher::PrefetcherStats> stats) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  std::string host = prefetcher->main_frame_url().host();
+  auto it = prefetches_.find(host);
+  // Can happen after Shutdown() has been called, since it purges |prefetches_|.
+  if (it == prefetches_.end())
+    return;
+
+  DCHECK(it->second.first.get() == prefetcher);
+  stats_collector_->RecordPrefetcherStats(std::move(stats));
+
+  if (observer_)
+    observer_->OnPrefetchingFinished(prefetcher->main_frame_url());
+
+  // ResourcePrefetcher must be destroyed on the IO thread.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::BindOnce([](std::unique_ptr<ResourcePrefetcher>) {},
+                     base::Passed(std::move(it->second.first))));

[alexilin, 2017/07/03 14:55:01]

Just curious, do you know whether we really need to use base::Passed()? I've
landed a CL recently where I used just std::move() without base::Passed() to
pass a unique_ptr to another thread.

[Benoit L, 2017/07/03 15:43:14]

Thanks for letting me know!
Done.

+
+  prefetches_.erase(it);
+}
+
+TestLoadingObserver::~TestLoadingObserver() {
+  predictor_->SetObserverForTesting(nullptr);
+}
+
+TestLoadingObserver::TestLoadingObserver(LoadingPredictor* predictor)
+    : predictor_(predictor) {
+  predictor_->SetObserverForTesting(this);
+}
+
 }  // namespace predictors