Port HistoryService::GetVisibleVisitCountToHost to CancelableTaskTracker

chrome/browser/safe_browsing/browser_feature_extractor.cc

 // Copyright (c) 2012 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/safe_browsing/browser_feature_extractor.h"
 
 #include <map>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/format_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
-#include "chrome/browser/common/cancelable_request.h"
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/browser_features.h"
 #include "chrome/browser/safe_browsing/client_side_detection_host.h"
 #include "chrome/browser/safe_browsing/database_manager.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
@@ skipping 140 matching lines @@
     WebContents* tab,
     ClientSideDetectionHost* host)
     : tab_(tab),
       host_(host),
       weak_factory_(this) {
   DCHECK(tab);
 }
 
 BrowserFeatureExtractor::~BrowserFeatureExtractor() {
   weak_factory_.InvalidateWeakPtrs();
-  // Delete all the pending extractions (delete callback and request objects).
-  STLDeleteContainerPairFirstPointers(pending_extractions_.begin(),
-                                      pending_extractions_.end());
-
-  // Also cancel all the pending history service queries.
-  HistoryService* history;
-  bool success = GetHistoryService(&history);
-  DCHECK(success || pending_queries_.size() == 0);
-  // Cancel all the pending history lookups and cleanup the memory.
-  for (PendingQueriesMap::iterator it = pending_queries_.begin();
-       it != pending_queries_.end(); ++it) {
-    if (history) {
-      history->CancelRequest(it->first);
-    }
-    ExtractionData& extraction = it->second;
-    delete extraction.first;  // delete request

[blundell, 2014/06/24 14:08:58]

now when the tracker is destroyed it will destroy all outstanding requests,
right?

[sdefresne, 2014/06/25 12:20:37]

It will invalidate all requests in flight, and since the ClientPhishingRequest
object is passed using base::Passed() then it is deleted. This is the way that
ClientMalwareRequest object lifetime is controlled.

-  }
-  pending_queries_.clear();
 }
 
 void BrowserFeatureExtractor::ExtractFeatures(const BrowseInfo* info,
                                               ClientPhishingRequest* request,
                                               const DoneCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(request);
   DCHECK(info);
   DCHECK_EQ(0U, request->url().find("http:"));
   DCHECK(!callback.is_null());
@@ skipping 37 matching lines @@
         std::string(), controller, url_index, info->url_redirects, request);
   }
   if (first_host_index != -1) {
     AddNavigationFeatures(features::kHostPrefix,
                           controller,
                           first_host_index,
                           info->host_redirects,
                           request);
   }
 
+  // The API doesn't take a scoped_ptr because the API gets mocked and we
+  // cannot mock an API that takes scoped_ptr as arguments.
+  scoped_ptr<ClientPhishingRequest> req(request);
+
   ExtractBrowseInfoFeatures(*info, request);
-  pending_extractions_[request] = callback;
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&BrowserFeatureExtractor::StartExtractFeatures,
-                 weak_factory_.GetWeakPtr(), request, callback));
+                 weak_factory_.GetWeakPtr(),
+                 base::Passed(&req),
+                 callback));
 }
 
 void BrowserFeatureExtractor::ExtractMalwareFeatures(
     BrowseInfo* info,
     ClientMalwareRequest* request,
     const MalwareDoneCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   // Grab the IPs because they might go away before we're done
@@ skipping 38 matching lines @@
     AddFeature(features::kSafeBrowsingThreatType,
                static_cast<double>(info.unsafe_resource->threat_type),
                request);
   }
   if (info.http_status_code != 0) {
     AddFeature(features::kHttpStatusCode, info.http_status_code, request);
   }
 }
 
 void BrowserFeatureExtractor::StartExtractFeatures(
-    ClientPhishingRequest* request,
+    scoped_ptr<ClientPhishingRequest> request,
     const DoneCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  size_t removed = pending_extractions_.erase(request);
-  DCHECK_EQ(1U, removed);
   HistoryService* history;
   if (!request || !request->IsInitialized() || !GetHistoryService(&history)) {
-    callback.Run(false, request);
+    callback.Run(false, request.Pass());
     return;
   }
-  // HistoryService::QueryURL migrated from CancelableRequestComsumer to
-  // CancelableRequestTracker and there is no Handle to associate to the
-  // request. Instead manage the request object lifetime by using a scoped_ptr
-  // and using base::Passed(). So if the asynchronous call is canceled, the
-  // request is deleted, otherwise the callback becomes the owner.
-  scoped_ptr<ClientPhishingRequest> owned_request(request);
   history->QueryURL(GURL(request->url()),
                     true /* wants_visits */,
                     base::Bind(&BrowserFeatureExtractor::QueryUrlHistoryDone,
                                base::Unretained(this),
-                               base::Passed(&owned_request),
+                               base::Passed(&request),
                                callback),
                     &cancelable_task_tracker_);
 }
 
 void BrowserFeatureExtractor::QueryUrlHistoryDone(
-    scoped_ptr<ClientPhishingRequest> owned_request,
+    scoped_ptr<ClientPhishingRequest> request,
     const DoneCallback& callback,
     bool success,
     const history::URLRow& row,
     const history::VisitVector& visits) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(owned_request);
+  DCHECK(request);
   DCHECK(!callback.is_null());
-  ClientPhishingRequest* request = owned_request.release();
   if (!success) {
     // URL is not found in the history.  In practice this should not
     // happen (unless there is a real error) because we just visited
     // that URL.
-    callback.Run(false, request);
+    callback.Run(false, request.Pass());
     return;
   }
   AddFeature(features::kUrlHistoryVisitCount,
              static_cast<double>(row.visit_count()),
-             request);
+             request.get());
 
   base::Time threshold = base::Time::Now() - base::TimeDelta::FromDays(1);
   int num_visits_24h_ago = 0;
   int num_visits_typed = 0;
   int num_visits_link = 0;
   for (history::VisitVector::const_iterator it = visits.begin();
        it != visits.end();
        ++it) {
     if (!content::PageTransitionIsMainFrame(it->transition)) {
       continue;
     }
     if (it->visit_time < threshold) {
       ++num_visits_24h_ago;
     }
     content::PageTransition transition = content::PageTransitionStripQualifier(
         it->transition);
     if (transition == content::PAGE_TRANSITION_TYPED) {
       ++num_visits_typed;
     } else if (transition == content::PAGE_TRANSITION_LINK) {
       ++num_visits_link;
     }
   }
   AddFeature(features::kUrlHistoryVisitCountMoreThan24hAgo,
              static_cast<double>(num_visits_24h_ago),
-             request);
+             request.get());
   AddFeature(features::kUrlHistoryTypedCount,
              static_cast<double>(num_visits_typed),
-             request);
+             request.get());
   AddFeature(features::kUrlHistoryLinkCount,
              static_cast<double>(num_visits_link),
-             request);
+             request.get());
 
   // Issue next history lookup for host visits.
   HistoryService* history;
   if (!GetHistoryService(&history)) {
-    callback.Run(false, request);
+    callback.Run(false, request.Pass());
     return;
   }
-  CancelableRequestProvider::Handle next_handle =
-      history->GetVisibleVisitCountToHost(
-          GURL(request->url()),
-          &request_consumer_,
-          base::Bind(&BrowserFeatureExtractor::QueryHttpHostVisitsDone,
-                     base::Unretained(this)));
-  StorePendingQuery(next_handle, request, callback);
+  history->GetVisibleVisitCountToHost(
+      GURL(request->url()),
+      base::Bind(&BrowserFeatureExtractor::QueryHttpHostVisitsDone,
+                 base::Unretained(this),
+                 base::Passed(&request),
+                 callback),
+      &cancelable_task_tracker_);
 }
 
 void BrowserFeatureExtractor::QueryHttpHostVisitsDone(
-    CancelableRequestProvider::Handle handle,
+    scoped_ptr<ClientPhishingRequest> request,
+    const DoneCallback& callback,
     bool success,
     int num_visits,
     base::Time first_visit) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  ClientPhishingRequest* request;
-  DoneCallback callback;
-  if (!GetPendingQuery(handle, &request, &callback)) {
-    DLOG(FATAL) << "No pending history query found";
-    return;
-  }
   DCHECK(request);
   DCHECK(!callback.is_null());
   if (!success) {
-    callback.Run(false, request);
+    callback.Run(false, request.Pass());
     return;
   }
-  SetHostVisitsFeatures(num_visits, first_visit, true, request);
+  SetHostVisitsFeatures(num_visits, first_visit, true, request.get());
 
   // Same lookup but for the HTTPS URL.
   HistoryService* history;
   if (!GetHistoryService(&history)) {
-    callback.Run(false, request);
+    callback.Run(false, request.Pass());
     return;
   }
   std::string https_url = request->url();
-  CancelableRequestProvider::Handle next_handle =
-      history->GetVisibleVisitCountToHost(
-          GURL(https_url.replace(0, 5, "https:")),
-          &request_consumer_,
-          base::Bind(&BrowserFeatureExtractor::QueryHttpsHostVisitsDone,
-                     base::Unretained(this)));
-  StorePendingQuery(next_handle, request, callback);
+  history->GetVisibleVisitCountToHost(
+      GURL(https_url.replace(0, 5, "https:")),
+      base::Bind(&BrowserFeatureExtractor::QueryHttpsHostVisitsDone,
+                 base::Unretained(this),
+                 base::Passed(&request),
+                 callback),
+      &cancelable_task_tracker_);
 }
 
 void BrowserFeatureExtractor::QueryHttpsHostVisitsDone(
-    CancelableRequestProvider::Handle handle,
+    scoped_ptr<ClientPhishingRequest> request,
+    const DoneCallback& callback,
     bool success,
     int num_visits,
     base::Time first_visit) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  ClientPhishingRequest* request;
-  DoneCallback callback;
-  if (!GetPendingQuery(handle, &request, &callback)) {
-    DLOG(FATAL) << "No pending history query found";
-    return;
-  }
   DCHECK(request);
   DCHECK(!callback.is_null());
   if (!success) {
-    callback.Run(false, request);
+    callback.Run(false, request.Pass());
     return;
   }
-  SetHostVisitsFeatures(num_visits, first_visit, false, request);
-  callback.Run(true, request);  // We're done with all the history lookups.
+  SetHostVisitsFeatures(num_visits, first_visit, false, request.get());
+  callback.Run(true, request.Pass());
 }
 
 void BrowserFeatureExtractor::SetHostVisitsFeatures(
     int num_visits,
     base::Time first_visit,
     bool is_http_query,
     ClientPhishingRequest* request) {
   DCHECK(request);
   AddFeature(is_http_query ?
              features::kHttpHostVisitCount : features::kHttpsHostVisitCount,
              static_cast<double>(num_visits),
              request);
   if (num_visits > 0) {
     AddFeature(
         is_http_query ?
         features::kFirstHttpHostVisitMoreThan24hAgo :
         features::kFirstHttpsHostVisitMoreThan24hAgo,
         (first_visit < (base::Time::Now() - base::TimeDelta::FromDays(1))) ?
         1.0 : 0.0,
         request);
   }
 }
 
-void BrowserFeatureExtractor::StorePendingQuery(
-    CancelableRequestProvider::Handle handle,
-    ClientPhishingRequest* request,
-    const DoneCallback& callback) {
-  DCHECK_EQ(0U, pending_queries_.count(handle));
-  pending_queries_[handle] = std::make_pair(request, callback);
-}
-
-bool BrowserFeatureExtractor::GetPendingQuery(
-    CancelableRequestProvider::Handle handle,
-    ClientPhishingRequest** request,
-    DoneCallback* callback) {
-  PendingQueriesMap::iterator it = pending_queries_.find(handle);
-  DCHECK(it != pending_queries_.end());
-  if (it != pending_queries_.end()) {
-    *request = it->second.first;
-    *callback = it->second.second;
-    pending_queries_.erase(it);
-    return true;
-  }
-  return false;
-}
-
 bool BrowserFeatureExtractor::GetHistoryService(HistoryService** history) {
   *history = NULL;
   if (tab_ && tab_->GetBrowserContext()) {
     Profile* profile = Profile::FromBrowserContext(tab_->GetBrowserContext());
     *history = HistoryServiceFactory::GetForProfile(profile,
                                                     Profile::EXPLICIT_ACCESS);
     if (*history) {
       return true;
     }
   }
@@ skipping 14 matching lines @@
     // Limit the number of matched bad IPs in one request to control
     // the request's size
     if (matched_bad_ips >= kMaxMalwareIPPerRequest) {
       break;
     }
   }
   callback.Run(true, request.Pass());
 }
 
 }  // namespace safe_browsing