Mostly integrate new malware IP blacklist with the csd client. When

chrome/browser/safe_browsing/browser_feature_extractor.cc

Index: chrome/browser/safe_browsing/browser_feature_extractor.cc
diff --git a/chrome/browser/safe_browsing/browser_feature_extractor.cc b/chrome/browser/safe_browsing/browser_feature_extractor.cc
index 29e70336f998fb1e83251d79f797594e49c23d47..f724e66c573c45aa72475ce5849e50b0006a45b7 100644
--- a/chrome/browser/safe_browsing/browser_feature_extractor.cc
+++ b/chrome/browser/safe_browsing/browser_feature_extractor.cc
@@ -19,7 +19,8 @@
 #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_service.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"
@@ -35,7 +36,27 @@ using content::WebContents;
 
 namespace safe_browsing {
 
-const int BrowserFeatureExtractor::kMaxMalwareIPPerRequest = 5;
+namespace {
+
+const int kMaxMalwareIPPerRequest = 5;
+
+void FilterBenignIpsOnIOThread(
+    scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
+    IPUrlMap* ips) {

[mattm, 2013/10/31 05:26:40]

could add  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

[noé, 2013/10/31 20:39:58]

Done.

+  std::vector<std::string> to_delete;
+  for (IPUrlMap::const_iterator it = ips->begin();
+       it != ips->end(); ++it) {
+    if (!database_manager.get() ||
+        !database_manager->MatchMalwareIP(it->first)) {
+      to_delete.push_back(it->first);

[mattm, 2013/10/31 05:26:40]

It should be possible to do this without the second loop.. if you make a copy of
the iterator, then advance it, and then use the copy of the old location to do
the erase.

[noé, 2013/10/31 20:39:58]

Actually, we don't even need a copy iterator.  Done.

+    }
+  }
+  for (std::vector<std::string>::const_iterator it = to_delete.begin();
+       it != to_delete.end(); ++it) {
+    ips->erase(*it);
+  }
+}
+}  // namespace
 
 BrowseInfo::BrowseInfo() : http_status_code(0) {}
 
@@ -132,9 +153,9 @@ static void AddNavigationFeatures(
 
 BrowserFeatureExtractor::BrowserFeatureExtractor(
     WebContents* tab,
-    ClientSideDetectionService* service)
+    ClientSideDetectionHost* host)
     : tab_(tab),
-      service_(service),
+      host_(host),
       weak_factory_(this) {
   DCHECK(tab);
 }
@@ -169,11 +190,6 @@ void BrowserFeatureExtractor::ExtractFeatures(const BrowseInfo* info,
   DCHECK(info);
   DCHECK_EQ(0U, request->url().find("http:"));
   DCHECK(!callback.is_null());
-  if (callback.is_null()) {
-    DLOG(ERROR) << "ExtractFeatures called without a callback object";
-    return;
-  }
-
   // Extract features pertaining to this navigation.
   const NavigationController& controller = tab_->GetController();
   int url_index = -1;
@@ -230,42 +246,39 @@ void BrowserFeatureExtractor::ExtractFeatures(const BrowseInfo* info,
 }
 
 void BrowserFeatureExtractor::ExtractMalwareFeatures(
-    const BrowseInfo* info,
-    ClientMalwareRequest* request) {
+    BrowseInfo* info,
+    ClientMalwareRequest* request,
+    const MalwareDoneCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(request);
-  DCHECK(info);
   DCHECK_EQ(0U, request->url().find("http:"));
-  // get the IPs and urls that match the malware blacklisted IP list.
-  if (service_) {
-    int matched_bad_ips = 0;
-    for (IPUrlMap::const_iterator it = info->ips.begin();
-         it != info->ips.end(); ++it) {
-      if (service_->IsBadIpAddress(it->first)) {
-        AddMalwareFeature(features::kBadIpFetch + it->first,
-                          it->second, 1.0, request);
-        ++matched_bad_ips;
-        // Limit the number of matched bad IPs in one request to control
-        // the request's size
-        if (matched_bad_ips >= kMaxMalwareIPPerRequest) {
-          return;
-        }
-      }
-    }
-  }
+  DCHECK(!callback.is_null());
+
+  // Copy the IPs because they might go away before we're done

[mattm, 2013/10/31 05:26:40]

s/Copy/Grab/ (or something)

[noé, 2013/10/31 20:39:58]

Done.

+  // checking them against the IP blacklist on the IO thread.
+  scoped_ptr<IPUrlMap> ips(new IPUrlMap);
+  ips->swap(info->ips);
+
+  IPUrlMap* ips_ptr = ips.get();
+
+  // 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<ClientMalwareRequest> req(request);
+
+  // IP blacklist lookups have to happen on the IO thread.
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&FilterBenignIpsOnIOThread,
+                 host_->database_manager(),
+                 ips_ptr),
+      base::Bind(&BrowserFeatureExtractor::FinishExtractMalwareFeatures,
+                 weak_factory_.GetWeakPtr(),
+                 base::Passed(&ips), callback, base::Passed(&req)));
 }
 
 void BrowserFeatureExtractor::ExtractBrowseInfoFeatures(
     const BrowseInfo& info,
     ClientPhishingRequest* request) {
-  if (service_) {
-    for (IPUrlMap::const_iterator it = info.ips.begin();
-         it != info.ips.end(); ++it) {
-      if (service_->IsBadIpAddress(it->first)) {
-        AddFeature(features::kBadIpFetch + it->first, 1.0, request);
-      }
-    }
-  }
   if (info.unsafe_resource.get()) {
     // A SafeBrowsing interstitial was shown for the current URL.
     AddFeature(features::kSafeBrowsingMaliciousUrl +
@@ -495,4 +508,23 @@ bool BrowserFeatureExtractor::GetHistoryService(HistoryService** history) {
   return false;
 }
 
+void BrowserFeatureExtractor::FinishExtractMalwareFeatures(
+    scoped_ptr<IPUrlMap> bad_ips,
+    MalwareDoneCallback callback,
+    scoped_ptr<ClientMalwareRequest> request) {

[mattm, 2013/10/31 05:26:40]

could add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[noé, 2013/10/31 20:39:58]

Done.

+  int matched_bad_ips = 0;
+  for (IPUrlMap::const_iterator it = bad_ips->begin();
+       it != bad_ips->end(); ++it) {
+    AddMalwareFeature(features::kBadIpFetch + it->first,
+                      it->second, 1.0, request.get());
+    ++matched_bad_ips;
+    // 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