Loading Predictor: Use more RAII in PrefetchManager classes.

This refactoring simplifies some manual bookkeeping. It also is
helpful for adding the Stop() function, in the next commit.

Bug: 1092329
Change-Id: I34494155ea161d3e2a0358b6b3d09b40aef85fac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2257089Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781183}

chrome/browser/predictors/prefetch_manager.cc

@@ -66,26 +66,40 @@ PrefetchStats::PrefetchStats(const GURL& url)
     : url(url), start_time(base::TimeTicks::Now()) {}
 PrefetchStats::~PrefetchStats() = default;
 
-PrefetchInfo::PrefetchInfo(const GURL& url, size_t count)
-    : url(url),
-      queued_count(count),
-      stats(std::make_unique<PrefetchStats>(url)) {
+PrefetchInfo::PrefetchInfo(const GURL& url, PrefetchManager& manager)
+    : url(url), stats(std::make_unique<PrefetchStats>(url)), manager(&manager) {
   DCHECK(url.is_valid());
   DCHECK(url.SchemeIsHTTPOrHTTPS());
 }
 
 PrefetchInfo::~PrefetchInfo() = default;
 
-PrefetchJob::PrefetchJob(PrefetchRequest prefetch_request, PrefetchInfo* info)
+void PrefetchInfo::OnJobCreated() {
+  job_count++;
+}
+
+void PrefetchInfo::OnJobDestroyed() {
+  job_count--;
+  if (is_done()) {
+    // Destroys |this|.
+    manager->AllPrefetchJobsForUrlFinished(*this);
+  }
+}
+
+PrefetchJob::PrefetchJob(PrefetchRequest prefetch_request, PrefetchInfo& info)
     : url(prefetch_request.url),
       network_isolation_key(std::move(prefetch_request.network_isolation_key)),
-      info(info) {
+      info(info.weak_factory.GetWeakPtr()) {
   DCHECK(url.is_valid());
   DCHECK(url.SchemeIsHTTPOrHTTPS());
   DCHECK(network_isolation_key.IsFullyPopulated());
+  info.OnJobCreated();
 }
 
-PrefetchJob::~PrefetchJob() = default;
+PrefetchJob::~PrefetchJob() {
+  if (info)
+    info->OnJobDestroyed();
+}
 
 PrefetchManager::PrefetchManager(base::WeakPtr<Delegate> delegate,
                                  Profile* profile)
@@ -103,37 +117,32 @@ void PrefetchManager::Start(const GURL& url,
 
   PrefetchInfo* info;
   if (prefetch_info_.find(url) == prefetch_info_.end()) {
-    auto iterator_and_whether_inserted = prefetch_info_.emplace(
-        url, std::make_unique<PrefetchInfo>(url, requests.size()));
+    auto iterator_and_whether_inserted =
+        prefetch_info_.emplace(url, std::make_unique<PrefetchInfo>(url, *this));
     info = iterator_and_whether_inserted.first->second.get();
   } else {
     info = prefetch_info_.find(url)->second.get();
-    info->queued_count += requests.size();
   }
 
   for (auto& request : requests) {
-    PrefetchJobId job_id =
-        jobs_.Add(std::make_unique<PrefetchJob>(std::move(request), info));
-    queued_jobs_.push_back(job_id);
+    queued_jobs_.emplace_back(
+        std::make_unique<PrefetchJob>(std::move(request), *info));
   }
 
   TryToLaunchPrefetchJobs();
 }
 
-size_t PrefetchManager::inflight_jobs_count() const {
-  DCHECK_GE(jobs_.size(), queued_jobs_.size());
-  return jobs_.size() - queued_jobs_.size();
-}
-
-void PrefetchManager::PrefetchUrl(PrefetchInfo& info,
-                                  const GURL& prefetch_url,
-                                  PrefetchJobId job_id,
+void PrefetchManager::PrefetchUrl(std::unique_ptr<PrefetchJob> job,
                                   network::SharedURLLoaderFactory& factory) {
+  DCHECK(job);
+  DCHECK(job->info);
+
+  PrefetchInfo& info = *job->info;
   url::Origin top_frame_origin = url::Origin::Create(info.url);
 
   network::ResourceRequest request;
   request.method = "GET";
-  request.url = prefetch_url;
+  request.url = job->url;
   request.site_for_cookies = net::SiteForCookies::FromUrl(info.url);
   request.request_initiator = top_frame_origin;
   request.referrer = info.url;
@@ -176,7 +185,7 @@ void PrefetchManager::PrefetchUrl(PrefetchInfo& info,
       network::mojom::kURLLoadOptionNone, request, std::move(url_loader_client),
       net::MutableNetworkTrafficAnnotationTag(kPrefetchTrafficAnnotation));
 
-  ++info.inflight_count;
+  ++inflight_jobs_count_;
 
   // The idea of prefetching is for the network service to put the response in
   // the http cache. So from the prefetching layer, nothing needs to be done
@@ -184,29 +193,13 @@ void PrefetchManager::PrefetchUrl(PrefetchInfo& info,
   network::EmptyURLLoaderClient::DrainURLRequest(
       std::move(client_receiver), std::move(loader),
       base::BindOnce(&PrefetchManager::OnPrefetchFinished,
-                     weak_factory_.GetWeakPtr(), job_id));
-}
-
-PrefetchInfo* PrefetchManager::GetJobInfo(PrefetchJobId job_id) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  PrefetchJob* job = jobs_.Lookup(job_id);
-  DCHECK(job);
-  return job->info;
+                     weak_factory_.GetWeakPtr(), std::move(job)));
 }
 
-void PrefetchManager::OnPrefetchFinished(PrefetchJobId job_id) {
+void PrefetchManager::OnPrefetchFinished(std::unique_ptr<PrefetchJob> job) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Get the info before destroying the job.
-  PrefetchInfo* info = GetJobInfo(job_id);
-  jobs_.Remove(job_id);
-
-  // Clean up the job and start more if needed.
-  DCHECK_GT(info->inflight_count, 0u);
-  --info->inflight_count;
-  if (info->is_done())
-    AllPrefetchJobsForUrlFinished(info);
+  --inflight_jobs_count_;
   TryToLaunchPrefetchJobs();
 }
 
@@ -221,32 +214,26 @@ void PrefetchManager::TryToLaunchPrefetchJobs() {
   scoped_refptr<network::SharedURLLoaderFactory> factory =
       storage_partition->GetURLLoaderFactoryForBrowserProcess();
 
-  while (!queued_jobs_.empty() && inflight_jobs_count() < kMaxInflightJobs) {
-    auto job_id = queued_jobs_.front();
+  while (!queued_jobs_.empty() && inflight_jobs_count_ < kMaxInflightJobs) {
+    std::unique_ptr<PrefetchJob> job = std::move(queued_jobs_.front());
     queued_jobs_.pop_front();
-    PrefetchJob* job = jobs_.Lookup(job_id);
-    DCHECK(job);
-    PrefetchInfo* info = job->info;
+    base::WeakPtr<PrefetchInfo> info = job->info;
+    // |this| owns all infos.
+    DCHECK(info);
 
     if (job->url.is_valid() && factory)
-      PrefetchUrl(*info, job->url, job_id, *factory);
-
-    DCHECK_LE(1u, info->queued_count);
-    --info->queued_count;
-    if (info->is_done())
-      AllPrefetchJobsForUrlFinished(info);
+      PrefetchUrl(std::move(job), *factory);
   }
 }
 
-void PrefetchManager::AllPrefetchJobsForUrlFinished(PrefetchInfo* info) {
-  DCHECK(info);
-  DCHECK(info->is_done());
-  auto it = prefetch_info_.find(info->url);
+void PrefetchManager::AllPrefetchJobsForUrlFinished(PrefetchInfo& info) {
+  DCHECK(info.is_done());
+  auto it = prefetch_info_.find(info.url);
   DCHECK(it != prefetch_info_.end());
-  DCHECK(info == it->second.get());
+  DCHECK(&info == it->second.get());
 
   if (delegate_)
-    delegate_->PrefetchFinished(std::move(info->stats));
+    delegate_->PrefetchFinished(std::move(info.stats));
   prefetch_info_.erase(it);
 }
 

chrome/browser/predictors/prefetch_manager.h

@@ -26,6 +26,7 @@ class SharedURLLoaderFactory;
 namespace predictors {
 
 struct PrefetchRequest;
+class PrefetchManager;
 
 struct PrefetchStats {
   explicit PrefetchStats(const GURL& url);
@@ -42,23 +43,30 @@ struct PrefetchStats {
 
 // Stores the status of all prefetches associated with a given |url|.
 struct PrefetchInfo {
-  PrefetchInfo(const GURL& url, size_t count);
+  PrefetchInfo(const GURL& url, PrefetchManager& manager);
   ~PrefetchInfo();
 
   PrefetchInfo(const PrefetchInfo&) = delete;
   PrefetchInfo& operator=(const PrefetchInfo&) = delete;
 
-  bool is_done() const { return queued_count == 0 && inflight_count == 0; }
+  // Called by PrefetchJob only.
+  void OnJobCreated();
+  void OnJobDestroyed();
+
+  bool is_done() const { return job_count == 0; }
 
   GURL url;
-  size_t queued_count = 0;
-  size_t inflight_count = 0;
+  size_t job_count = 0;
   std::unique_ptr<PrefetchStats> stats;
+  // Owns |this|.
+  PrefetchManager* const manager;
+
+  base::WeakPtrFactory<PrefetchInfo> weak_factory{this};
 };
 
 // Stores all data need for running a prefetch to a |url|.
 struct PrefetchJob {
-  PrefetchJob(PrefetchRequest prefetch_request, PrefetchInfo* info);
+  PrefetchJob(PrefetchRequest prefetch_request, PrefetchInfo& info);
   ~PrefetchJob();
 
   PrefetchJob(const PrefetchJob&) = delete;
@@ -66,10 +74,8 @@ struct PrefetchJob {
 
   GURL url;
   net::NetworkIsolationKey network_isolation_key;
-
-  // Danger: this is a raw pointer that PrefetchJob can outlive. It must be
-  // accessed from PrefetchManager only, which owns PrefetchInfo.
-  PrefetchInfo* info;
+  // PrefetchJob can outlive PrefetchInfo.
+  base::WeakPtr<PrefetchInfo> info;
 };
 
 // PrefetchManager prefetches input lists of URLs.
@@ -95,52 +101,46 @@ class PrefetchManager {
   static const size_t kMaxInflightJobs = 3;
 
   PrefetchManager(base::WeakPtr<Delegate> delegate, Profile* profile);
-  virtual ~PrefetchManager();
+  ~PrefetchManager();
 
   PrefetchManager(const PrefetchManager&) = delete;
   PrefetchManager& operator=(const PrefetchManager&) = delete;
 
   // Starts prefetch jobs keyed by |url|.
-  virtual void Start(const GURL& url, std::vector<PrefetchRequest> requests);
+  void Start(const GURL& url, std::vector<PrefetchRequest> requests);
 
-  // TODO(falken): Add a Stop() method like PreconnectManager.
-  // virtual void Stop(const GURL& url);
+  // TODO(falken): Implement Stop().
+  // void Stop(const GURL& url);
 
   base::WeakPtr<PrefetchManager> GetWeakPtr() {
     return weak_factory_.GetWeakPtr();
   }
 
+  // Called by PrefetchInfo.
+  void AllPrefetchJobsForUrlFinished(PrefetchInfo& info);
+
  private:
-  using PrefetchJobMap = base::IDMap<std::unique_ptr<PrefetchJob>>;
-  using PrefetchJobId = PrefetchJobMap::KeyType;
   friend class PrefetchManagerTest;
 
-  // The total number of prefetches that have started and not yet finished,
-  // across all main frame URLs.
-  size_t inflight_jobs_count() const;
-
-  void PrefetchUrl(PrefetchInfo& info,
-                   const GURL& prefetch_url,
-                   PrefetchJobId job_id,
+  void PrefetchUrl(std::unique_ptr<PrefetchJob> job,
                    network::SharedURLLoaderFactory& factory);
-  PrefetchInfo* GetJobInfo(PrefetchJobId job_id);
-  void OnPrefetchFinished(PrefetchJobId job_id);
+  void OnPrefetchFinished(std::unique_ptr<PrefetchJob> job);
 
   void TryToLaunchPrefetchJobs();
-  void AllPrefetchJobsForUrlFinished(PrefetchInfo* info);
 
   base::WeakPtr<Delegate> delegate_;
   Profile* const profile_;
 
   // All the jobs that haven't yet started. A job is removed once it starts.
-  std::list<PrefetchJobId> queued_jobs_;
-
-  // All the jobs that haven't yet finished (including queued jobs). A job is
-  // removed once it finishes.
-  PrefetchJobMap jobs_;
+  // Inflight jobs destruct once finished.
+  std::list<std::unique_ptr<PrefetchJob>> queued_jobs_;
 
   std::map<GURL, std::unique_ptr<PrefetchInfo>> prefetch_info_;
 
+  // The total number of prefetches that have started and not yet finished,
+  // across all main frame URLs.
+  size_t inflight_jobs_count_ = 0;
+
   base::WeakPtrFactory<PrefetchManager> weak_factory_{this};
 };