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

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_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"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/page_transition_types.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
 using content::NavigationController;
 using content::NavigationEntry;
 using content::WebContents;
 
 namespace safe_browsing {
 
-const int BrowserFeatureExtractor::kMaxMalwareIPPerRequest = 5;
-
 BrowseInfo::BrowseInfo() : http_status_code(0) {}
 
 BrowseInfo::~BrowseInfo() {}
 
 static void AddFeature(const std::string& feature_name,
                        double feature_value,
                        ClientPhishingRequest* request) {
   DCHECK(request);
   ClientPhishingRequest::Feature* feature =
       request->add_non_model_feature_map();
@@ skipping 75 matching lines @@
                                   features::kRedirect,
                                   i,
                                   printable_redirect.c_str()),
                1.0,
                request);
   }
 }
 
 BrowserFeatureExtractor::BrowserFeatureExtractor(
     WebContents* tab,
-    ClientSideDetectionService* service)
+    ClientSideDetectionHost* host)
     : tab_(tab),
-      service_(service),
+      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());
 
@@ skipping 14 matching lines @@
 }
 
 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());
-  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;
   int first_host_index = -1;
 
   GURL request_url(request->url());
   int index = controller.GetCurrentEntryIndex();
   // The url that we are extracting features for should already be commited.
   DCHECK_NE(index, -1);
   for (; index >= 0; index--) {
@@ skipping 35 matching lines @@
   }
 
   ExtractBrowseInfoFeatures(*info, request);
   pending_extractions_[request] = callback;
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&BrowserFeatureExtractor::StartExtractFeatures,
                  weak_factory_.GetWeakPtr(), request, callback));
 }
 
-void BrowserFeatureExtractor::ExtractMalwareFeatures(
-    const BrowseInfo* info,
-    ClientMalwareRequest* request) {
-  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;
-        }
-      }
+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(
+    BrowseInfo* info,
+    ClientMalwareRequest* request,
+    const MalwareDoneCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(0U, request->url().find("http:"));
+  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 +
                info.unsafe_resource->url.spec(),
                1.0,
                request);
     AddFeature(features::kSafeBrowsingOriginalUrl +
                info.unsafe_resource->original_url.spec(),
                1.0,
                request);
@@ skipping 209 matching lines @@
     *history = HistoryServiceFactory::GetForProfile(profile,
                                                     Profile::EXPLICIT_ACCESS);
     if (*history) {
       return true;
     }
   }
   VLOG(2) << "Unable to query history.  No history service available.";
   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