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/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;
@@ skipping 92 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 75 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,
+    const 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;

[mattm, 2013/10/25 06:28:12]

I think remove this, the DCHECK should be fine. Chrome's philosophy is that it
does happen in a release build, we'd rather have a crash report than just have
things silently break. (At least, I think calling .Run on a null callback causes
a crash?)

[noé, 2013/10/28 23:39:26]

Done.

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

It looks like you removed the one from the other method :)

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

Removed it in both cases now :).

+  }
+  // Copy the IPs because they might go away before we're done
+  // checking them against the IP blacklist on the IO thread.
+  scoped_ptr<IPUrlMap> ips(new IPUrlMap(info.ips.begin(),
+                                        info.ips.end()));

[mattm, 2013/10/25 06:28:12]

If the ips map isn't used by any other functions, maybe just do a .swap with an
empty map?

[noé, 2013/10/28 23:39:26]

Done.

+
+  // IP blacklist lookups have to happen on the IO thread.
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&BrowserFeatureExtractor::StartExtractMalwareFeatures,
+                 weak_factory_.GetWeakPtr(),

[mattm, 2013/10/25 06:28:12]

weakptrs shouldn't be de-referenced on a different thread. Have you tried this
in a debug build? In any case, even if the weakptr was valid when
StartExtractMalwareFeatures ran, the object could still be deleted at any point
on the UI thread while the function was still running on the IO thread.


I think what you could do here is make ExtractMalwareFeaturesOnIOThread an
anonymous function, pass in the database_manager_ (it's refcountedthreadsafe),
and pass in the copy of the ips map, and pass in a callback to something like
"FinishExtractMalwareFeatures". That callback could safely use a weakptr,
because it would only be de-reffed on the UI thread.

The ExtractMalwareFeaturesOnIOThread function could then go through the map,
remove any non-blacklisted entries, and post the ip map back to
FinishExtractMalwareFeatures on the UI thread.

FinishExtractMalwareFeatures would go through the ip map and do the actual
AddMalwareFeature calls.

(Actually you could use PostTaskAndReply for this instead of having
ExtractMalwareFeaturesOnIOThread manually post back.)

[noé, 2013/10/28 23:39:26]

Thank you very much for providing that example.  That was super useful. Done.

+                 base::Passed(&ips), request, callback));
+}
+
+static void CallMalwareCallback(

[mattm, 2013/10/25 06:28:12]

Chromium style is to use anonymous namespaces rather than static functions.

[noé, 2013/10/28 23:39:26]

Done.

+    const BrowserFeatureExtractor::MalwareDoneCallback& callback,
+    bool success,
     ClientMalwareRequest* request) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(request);
-  DCHECK(info);
-  DCHECK_EQ(0U, request->url().find("http:"));
+  callback.Run(success, request);
+}
+
+void BrowserFeatureExtractor::StartExtractMalwareFeatures(

[mattm, 2013/10/25 06:28:12]

Maybe name ExtractMalwareFeaturesOnIOThread?

[noé, 2013/10/28 23:39:26]

Done.

+    scoped_ptr<IPUrlMap> ips,
+    ClientMalwareRequest* request,
+    const MalwareDoneCallback& callback) {
   // 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;
-        }
+  if (!host_) {
+    callback.Run(false, request);
+    return;
+  }
+  int matched_bad_ips = 0;
+  for (IPUrlMap::const_iterator it = ips->begin();
+       it != ips->end(); ++it) {
+    if (host_->IsBadIpAddress(it->first)) {
+      AddMalwareFeature(features::kBadIpFetch + it->first,
+                        it->second, 1.0, request);

[mattm, 2013/10/25 06:28:12]

I don't think AddMalwareFeature is threadsafe?

[noé, 2013/10/28 23:39:26]

Can you elaborate why you think it is not thread safe?  Are you concerned about
the request object?  We own it here so I don't think anybody else even knows
about this pointer.

In any case, I think this comment isn't relevant anymore since I only access the
request object on a single thread.

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

Oh, you're right. I missed that the request objects in ExtractMalwareFeatures
and ExtractFeatures are different objects.
indeed.

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

Done.

+      ++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;
       }
     }
   }
+  // We guanteed to the client of this API that the callback was going to
+  // be called on the UI thread.
+  BrowserThread::PostTask(
+      BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&CallMalwareCallback, callback, true, request));

[mattm, 2013/10/25 06:28:12]

You can just do base::Bind(callback, true, request)

[noé, 2013/10/28 23:39:26]

Neat!

 }
 
 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);

[mattm, 2013/10/25 06:28:12]

This doesn't exist in the new version, intentional?

[noé, 2013/10/28 23:39:26]

Good catch :-).  Yes, this is intentional.  We don't need it as part of the
phishing classifier anymore.  I'll re-add it if we need it again in the future.

-      }
-    }
-  }
   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 210 matching lines @@
                                                     Profile::EXPLICIT_ACCESS);
     if (*history) {
       return true;
     }
   }
   VLOG(2) << "Unable to query history.  No history service available.";
   return false;
 }
 
 }  // namespace safe_browsing