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..0b0a580aeeb8ce482283fa45a693ea3b9f918a59 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,8 +36,6 @@ using content::WebContents;
 
 namespace safe_browsing {
 
-const int BrowserFeatureExtractor::kMaxMalwareIPPerRequest = 5;
-
 BrowseInfo::BrowseInfo() : http_status_code(0) {}
 
 BrowseInfo::~BrowseInfo() {}
@@ -132,9 +131,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 +168,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;
@@ -229,43 +223,64 @@ void BrowserFeatureExtractor::ExtractFeatures(const BrowseInfo* info,
                  weak_factory_.GetWeakPtr(), request, callback));
 }
 
+namespace {
+
+const int kMaxMalwareIPPerRequest = 5;
+
+void FilterBenignIpsOnOnIOThread(

[mattm, 2013/10/29 01:11:47]

OnOn

[noé, 2013/10/31 02:41:12]

DoneDone.

[noé, 2013/10/31 02:41:12]

DoneDone.

+    scoped_refptr<RefCountedIPUrlMap> ips,
+    scoped_refptr<SafeBrowsingDatabaseManager> database_manager) {
+  IPUrlMap& ip_map = *(ips->data);
+  std::vector<std::string> to_delete;
+  for (IPUrlMap::const_iterator it = ip_map.begin();
+       it != ip_map.end(); ++it) {
+    if (!database_manager.get() ||
+        !database_manager->MatchMalwareIP(it->first)) {
+      to_delete.push_back(it->first);
+    }
+  }
+  for (std::vector<std::string>::const_iterator it = to_delete.begin();
+       it != to_delete.end(); ++it) {
+    ip_map.erase(*it);
+  }
+}
+}  // namespace

[mattm, 2013/10/29 01:11:47]

The chromium style seems to be to have the anonymous namespace at the top of the
file, though I don't think it's actually written anywhere..

[noé, 2013/10/31 02:41:12]

Done.

[noé, 2013/10/31 02:41:12]

Done.

+
 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());
+  if (callback.is_null()) {
+    DLOG(ERROR) << "ExtractMalwareFeatures called without a callback object";
+    return;
   }
+  // Copy the IPs because they might go away before we're done
+  // checking them against the IP blacklist on the IO thread.
+  scoped_refptr<RefCountedIPUrlMap> ips(new RefCountedIPUrlMap(new IPUrlMap));
+  ips->data->swap(info->ips);
+
+  // 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(

[mattm, 2013/10/29 01:11:47]

It should be possible to do this with ips still as a scoped_ptr instead of a
RefCountedData.  In the first bind call you can pass a raw ptr to it, in the
second call you can use base::Passed().  Ex: 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

(Do note the saving the raw ptr separately instead of calling
scoped_ptr_obj.get() in the first bind call.  base::Passed takes ownership and
clears the passed in variable, so if you call .get() in the first bind call, it
might be null due to undefined execution order.)

+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&FilterBenignIpsOnOnIOThread,
+                 ips,
+                 host_->database_manager()),
+      base::Bind(&BrowserFeatureExtractor::FinishExtractMalwareFeatures,
+                 weak_factory_.GetWeakPtr(),
+                 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 +510,25 @@ bool BrowserFeatureExtractor::GetHistoryService(HistoryService** history) {
   return false;
 }
 
+void BrowserFeatureExtractor::FinishExtractMalwareFeatures(
+    scoped_refptr<RefCountedIPUrlMap> bad_ips,
+    MalwareDoneCallback callback,
+    scoped_ptr<ClientMalwareRequest> request) {
+  int matched_bad_ips = 0;
+  const IPUrlMap& bad_ips_map = *(bad_ips->data);
+  for (IPUrlMap::const_iterator it = bad_ips_map.begin();
+       it != bad_ips_map.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;
+    }
+  }
+  bool success = true;

[mattm, 2013/10/29 01:11:47]

unnecessary?

[noé, 2013/10/31 02:41:12]

Done.

+  callback.Run(success, request.Pass());
+}
+
 }  // namespace safe_browsing