Separate pre-classification checks for client-side malware and phishing

chrome/browser/safe_browsing/client_side_detection_host.cc

Index: chrome/browser/safe_browsing/client_side_detection_host.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_host.cc b/chrome/browser/safe_browsing/client_side_detection_host.cc
index 130f0cad52a83d237858b5fe12738cb59e8e25ea..276c2d786e5769e643ec8a6b527752f63f3a96fb 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -19,8 +19,6 @@
 #include "chrome/browser/safe_browsing/client_side_detection_service.h"
 #include "chrome/browser/safe_browsing/database_manager.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
-#include "chrome/common/chrome_switches.h"
-#include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
 #include "chrome/common/safe_browsing/safebrowsing_messages.h"
@@ -36,6 +34,7 @@
 #include "content/public/browser/resource_request_details.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
+#include "content/public/common/url_constants.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
@@ -50,28 +49,34 @@ const int ClientSideDetectionHost::kMaxIPsPerBrowse = 200;
 
 const char kSafeBrowsingMatchKey[] = "safe_browsing_match";
 
+typedef base::Callback<void(bool)> ShouldClassifyUrlCallback;
+
 // This class is instantiated each time a new toplevel URL loads, and
-// asynchronously checks whether the phishing classifier should run for this
-// URL.  If so, it notifies the renderer with a StartPhishingDetection IPC.
-// Objects of this class are ref-counted and will be destroyed once nobody
-// uses it anymore.  If |web_contents|, |csd_service| or |host| go away you need
-// to call Cancel().  We keep the |database_manager| alive in a ref pointer for
-// as long as it takes.
+// asynchronously checks whether the malware and phishing classifiers should run
+// for this URL.  If so, it notifies the host class by calling the provided
+// callback form the UI thread.  Objects of this class are ref-counted and will
+// be destroyed once nobody uses it anymore.  If |web_contents|, |csd_service|
+// or |host| go away you need to call Cancel().  We keep the |database_manager|
+// alive in a ref pointer for as long as it takes.
 class ClientSideDetectionHost::ShouldClassifyUrlRequest
     : public base::RefCountedThreadSafe<
           ClientSideDetectionHost::ShouldClassifyUrlRequest> {
  public:
-  ShouldClassifyUrlRequest(const content::FrameNavigateParams& params,
-                           WebContents* web_contents,
-                           ClientSideDetectionService* csd_service,
-                           SafeBrowsingDatabaseManager* database_manager,
-                           ClientSideDetectionHost* host)
-      : canceled_(false),
-        params_(params),
+  ShouldClassifyUrlRequest(
+      const content::FrameNavigateParams& params,
+      const ShouldClassifyUrlCallback& start_phishing_classification,
+      const ShouldClassifyUrlCallback& start_malware_classification,
+      WebContents* web_contents,
+      ClientSideDetectionService* csd_service,
+      SafeBrowsingDatabaseManager* database_manager,
+      ClientSideDetectionHost* host)
+      : params_(params),
         web_contents_(web_contents),
         csd_service_(csd_service),
         database_manager_(database_manager),
-        host_(host) {
+        host_(host),
+        start_phishing_classification_cb_(start_phishing_classification),
+        start_malware_classification_cb_(start_malware_classification) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     DCHECK(web_contents_);
     DCHECK(csd_service_);
@@ -84,6 +89,7 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
 
     // We start by doing some simple checks that can run on the UI thread.
     UMA_HISTOGRAM_COUNTS("SBClientPhishing.ClassificationStart", 1);
+    UMA_HISTOGRAM_COUNTS("SBClientMalware.ClassificationStart", 1);

[Alexei Svitkine (slow), 2014/03/25 13:51:39]

Is there a plan for this to eventually be something other than 1?

It seems wasteful to allocate a 50-bucket counts histogram just to ever log a
value to one of those buckets.

[noé, 2014/03/25 15:54:25]

Mostly copy & paste from the other histogram above.  We don't expect to use the
other buckets.  What are you suggesting I should use instead?

[Alexei Svitkine (slow), 2014/03/25 16:43:03]

I suggest UMA_HISTOGRAM_BOOLEAN().

If you look at the bucket distribution for UMA_HISTOGRAM_COUNTS(), the bucket
for value 1 will have range [1-2).

(You can see this by looking at the data for another UMA_HISTOGRAM_COUNTS() that
logs a broad range of values, e.g.:
https://uma.googleplex.com/histograms/?dayCount=1&endDate=03-25-2014&version=...
)

Since the bucket only has size 1, you can change it to UMA_HISTOGRAM_BOOLEAN
which has two buckets - one for 0 and another for 1.

You can change both of these to it, since it should be backwards compatible in
terms of reported buckets to the old one if only 1 has ever been logged.

[noé, 2014/03/25 20:23:11]

Thanks for the tip.  I changed a handful of histograms that are in that file.

 
     // Only classify [X]HTML documents.
     if (params_.contents_mime_type != "text/html" &&
@@ -91,50 +97,53 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
       VLOG(1) << "Skipping phishing classification for URL: " << params_.url
               << " because it has an unsupported MIME type: "
               << params_.contents_mime_type;
-      UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
-                                NO_CLASSIFY_UNSUPPORTED_MIME_TYPE,
-                                NO_CLASSIFY_MAX);
-      return;
+      DontClassifyForPhishing(NO_CLASSIFY_UNSUPPORTED_MIME_TYPE);
     }
 
     if (csd_service_->IsPrivateIPAddress(params_.socket_address.host())) {
       VLOG(1) << "Skipping phishing classification for URL: " << params_.url
               << " because of hosting on private IP: "
               << params_.socket_address.host();
-      UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
-                                NO_CLASSIFY_PRIVATE_IP,
-                                NO_CLASSIFY_MAX);
-      return;
+      DontClassifyForPhishing(NO_CLASSIFY_PRIVATE_IP);
+      DontClassifyForMalware(NO_CLASSIFY_PRIVATE_IP);
     }
 
-    // Don't run the phishing classifier if the tab is incognito.
-    if (web_contents_->GetBrowserContext()->IsOffTheRecord()) {
+    // For phishing we only classify HTTP pages.
+    if (!params_.url.SchemeIs(content::kHttpScheme)) {
       VLOG(1) << "Skipping phishing classification for URL: " << params_.url
-              << " because we're browsing incognito.";
-      UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
-                                NO_CLASSIFY_OFF_THE_RECORD,
-                                NO_CLASSIFY_MAX);
+              << " because it is not HTTP: "
+              << params_.socket_address.host();
+      DontClassifyForPhishing(NO_CLASSIFY_NOT_HTTP_URL);
+    }
 
-      return;
+    // Don't run any classifier if the tab is incognito.
+    if (web_contents_->GetBrowserContext()->IsOffTheRecord()) {
+      VLOG(1) << "Skipping phishing and malware classification for URL: "
+              << params_.url << " because we're browsing incognito.";
+      DontClassifyForPhishing(NO_CLASSIFY_OFF_THE_RECORD);
+      DontClassifyForMalware(NO_CLASSIFY_OFF_THE_RECORD);
     }
 
     // We lookup the csd-whitelist before we lookup the cache because
     // a URL may have recently been whitelisted.  If the URL matches
-    // the csd-whitelist we won't start classification.  The
+    // the csd-whitelist we won't start phishing classification.  The
     // csd-whitelist check has to be done on the IO thread because it
     // uses the SafeBrowsing service class.
-    BrowserThread::PostTask(
-        BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&ShouldClassifyUrlRequest::CheckCsdWhitelist,
-                   this, params_.url));
+    if (ShouldClassifyForPhishing() || ShouldClassifyForMalware()) {
+      BrowserThread::PostTask(
+          BrowserThread::IO,
+          FROM_HERE,
+          base::Bind(&ShouldClassifyUrlRequest::CheckSafeBrowsingDatabase,
+                     this, params_.url));
+    }
   }
 
   void Cancel() {
-    canceled_ = true;
+    DontClassifyForPhishing(NO_CLASSIFY_CANCEL);
+    DontClassifyForMalware(NO_CLASSIFY_CANCEL);
     // Just to make sure we don't do anything stupid we reset all these
     // pointers except for the safebrowsing service class which may be
-    // accessed by CheckCsdWhitelist().
+    // accessed by CheckSafeBrowsingDatabase().
     web_contents_ = NULL;
     csd_service_ = NULL;
     host_ = NULL;
@@ -152,6 +161,11 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
     NO_CLASSIFY_MATCH_CSD_WHITELIST,
     NO_CLASSIFY_TOO_MANY_REPORTS,
     NO_CLASSIFY_UNSUPPORTED_MIME_TYPE,
+    NO_CLASSIFY_NO_DATABASE_MANAGER,
+    NO_CLASSIFY_KILLSWITCH,
+    NO_CLASSIFY_CANCEL,
+    NO_CLASSIFY_RESULT_FROM_CACHE,
+    NO_CLASSIFY_NOT_HTTP_URL,
 
     NO_CLASSIFY_MAX  // Always add new values before this one.
   };
@@ -159,49 +173,98 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
   // The destructor can be called either from the UI or the IO thread.
   virtual ~ShouldClassifyUrlRequest() { }
 
-  void CheckCsdWhitelist(const GURL& url) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    if (!database_manager_.get() ||
-        database_manager_->MatchCsdWhitelistUrl(url)) {
-      // We're done.  There is no point in going back to the UI thread.
-      VLOG(1) << "Skipping phishing classification for URL: " << url
-              << " because it matches the csd whitelist";
+  bool ShouldClassifyForPhishing() const {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    return !start_phishing_classification_cb_.is_null();
+  }
+
+  bool ShouldClassifyForMalware() const {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    return !start_malware_classification_cb_.is_null();
+  }
+
+  void DontClassifyForPhishing(PreClassificationCheckFailures reason) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (ShouldClassifyForPhishing()) {
+      // Track the first reason why we stopped classifying for phishing.
       UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
-                                NO_CLASSIFY_MATCH_CSD_WHITELIST,
-                                NO_CLASSIFY_MAX);
-      return;
+                                reason, NO_CLASSIFY_MAX);
+      DVLOG(2) << "Failed phishing pre-classification checks.  Reason: "
+               << reason;
+      start_phishing_classification_cb_.Run(false);
     }
+    start_phishing_classification_cb_.Reset();
+  }
 
-    bool malware_killswitch_on = database_manager_->IsMalwareKillSwitchOn();
+  void DontClassifyForMalware(PreClassificationCheckFailures reason) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (ShouldClassifyForMalware()) {
+      // Track the first reason why we stopped classifying for malware.
+      UMA_HISTOGRAM_ENUMERATION("SBClientMalware.PreClassificationCheckFail",
+                                reason, NO_CLASSIFY_MAX);
+      DVLOG(2) << "Failed malware pre-classification checks.  Reason: "
+               << reason;
+      start_malware_classification_cb_.Run(false);
+    }
+    start_malware_classification_cb_.Reset();
+  }
 
+  void CheckSafeBrowsingDatabase(const GURL& url) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    // We don't want to call the classification callbacks from the IO
+    // thread so we simply pass the results of this method to CheckCache()
+    // which is called on the UI thread;
+    PreClassificationCheckFailures phishing_reason = NO_CLASSIFY_MAX;
+    PreClassificationCheckFailures malware_reason = NO_CLASSIFY_MAX;
+    if (!database_manager_.get()) {
+      // We cannot check the Safe Browsing whitelists so we stop here
+      // for safety.
+      malware_reason = phishing_reason = NO_CLASSIFY_NO_DATABASE_MANAGER;
+    } else {
+      if (database_manager_->MatchCsdWhitelistUrl(url)) {
+        VLOG(1) << "Skipping phishing classification for URL: " << url
+                << " because it matches the csd whitelist";
+        phishing_reason = NO_CLASSIFY_MATCH_CSD_WHITELIST;
+      }
+      if (database_manager_->IsMalwareKillSwitchOn()) {
+        malware_reason = NO_CLASSIFY_KILLSWITCH;
+      }
+    }
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
-        base::Bind(&ShouldClassifyUrlRequest::CheckCache, this,
-                   malware_killswitch_on));
+        base::Bind(&ShouldClassifyUrlRequest::CheckCache,
+                   this,
+                   phishing_reason,
+                   malware_reason));
   }
 
-  void CheckCache(bool malware_killswitch_on) {
+  void CheckCache(PreClassificationCheckFailures phishing_reason,
+                  PreClassificationCheckFailures malware_reason) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    if (canceled_) {
-      return;
+    if (phishing_reason != NO_CLASSIFY_MAX)
+      DontClassifyForPhishing(phishing_reason);
+    if (malware_reason != NO_CLASSIFY_MAX)
+      DontClassifyForMalware(malware_reason);
+    if (!ShouldClassifyForMalware() && !ShouldClassifyForPhishing()) {
+      return;  // No point in doing anything else.
     }
-
-    host_->SetMalwareKillSwitch(malware_killswitch_on);
-    // If result is cached, we don't want to run classification again
+    // If result is cached, we don't want to run classification again.
+    // In that case we're just trying to show the warning.
     bool is_phishing;
     if (csd_service_->GetValidCachedResult(params_.url, &is_phishing)) {
       VLOG(1) << "Satisfying request for " << params_.url << " from cache";
       UMA_HISTOGRAM_COUNTS("SBClientPhishing.RequestSatisfiedFromCache", 1);
       // Since we are already on the UI thread, this is safe.
       host_->MaybeShowPhishingWarning(params_.url, is_phishing);
-      return;
+      DontClassifyForPhishing(NO_CLASSIFY_RESULT_FROM_CACHE);
     }
 
     // We want to limit the number of requests, though we will ignore the
     // limit for urls in the cache.  We don't want to start classifying
     // too many pages as phishing, but for those that we already think are
-    // phishing we want to give ourselves a chance to fix false positives.
+    // phishing we want to send a request to the server to give ourselves
+    // a chance to fix misclassifications.
     if (csd_service_->IsInCache(params_.url)) {
       VLOG(1) << "Reporting limit skipped for " << params_.url
               << " as it was in the cache.";
@@ -209,25 +272,21 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
     } else if (csd_service_->OverPhishingReportLimit()) {
       VLOG(1) << "Too many report phishing requests sent recently, "
               << "not running classification for " << params_.url;
-      UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
-                                NO_CLASSIFY_TOO_MANY_REPORTS,
-                                NO_CLASSIFY_MAX);
-      return;
+      DontClassifyForPhishing(NO_CLASSIFY_TOO_MANY_REPORTS);
+    }
+    if (csd_service_->OverMalwareReportLimit()) {
+      DontClassifyForMalware(NO_CLASSIFY_TOO_MANY_REPORTS);
     }
 
     // Everything checks out, so start classification.
     // |web_contents_| is safe to call as we will be destructed
     // before it is.
-    VLOG(1) << "Instruct renderer to start phishing detection for URL: "
-            << params_.url;
-    content::RenderViewHost* rvh = web_contents_->GetRenderViewHost();
-    rvh->Send(new SafeBrowsingMsg_StartPhishingDetection(
-        rvh->GetRoutingID(), params_.url));
+    if (ShouldClassifyForPhishing())
+      start_phishing_classification_cb_.Run(true);
+    if (ShouldClassifyForMalware())
+      start_malware_classification_cb_.Run(true);
   }
 
-  // No need to protect |canceled_| with a lock because it is only read and
-  // written by the UI thread.
-  bool canceled_;
   content::FrameNavigateParams params_;
   WebContents* web_contents_;
   ClientSideDetectionService* csd_service_;
@@ -236,6 +295,9 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
   scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
   ClientSideDetectionHost* host_;
 
+  ShouldClassifyUrlCallback start_phishing_classification_cb_;
+  ShouldClassifyUrlCallback start_malware_classification_cb_;
+
   DISALLOW_COPY_AND_ASSIGN(ShouldClassifyUrlRequest);
 };
 
@@ -248,10 +310,12 @@ ClientSideDetectionHost* ClientSideDetectionHost::Create(
 ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
     : content::WebContentsObserver(tab),
       csd_service_(NULL),
+      classification_request_(NULL),
+      should_extract_malware_features_(true),
+      should_classify_for_malware_(false),
+      onload_complete_(false),
       weak_factory_(this),
-      unsafe_unique_page_id_(-1),
-      malware_killswitch_on_(false),
-      malware_report_enabled_(false) {
+      unsafe_unique_page_id_(-1) {
   DCHECK(tab);
   // Note: csd_service_ and sb_service will be NULL here in testing.
   csd_service_ = g_browser_process->safe_browsing_detection_service();
@@ -266,13 +330,6 @@ ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
     database_manager_ = sb_service->database_manager();
     ui_manager_->AddObserver(this);
   }
-
-  // Only enable the malware bad IP matching and report feature for canary
-  // and dev channel.
-  chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
-  malware_report_enabled_ = (
-      channel == chrome::VersionInfo::CHANNEL_DEV ||
-      channel == chrome::VersionInfo::CHANNEL_CANARY);
 }
 
 ClientSideDetectionHost::~ClientSideDetectionHost() {
@@ -302,6 +359,10 @@ void ClientSideDetectionHost::DidNavigateMainFrame(
     // begin a new classification.
     return;
   }
+  // Cancel any pending classification request.
+  if (classification_request_.get()) {
+    classification_request_->Cancel();
+  }
   // If we navigate away and there currently is a pending phishing
   // report request we have to cancel it to make sure we don't display
   // an interstitial for the wrong page.  Note that this won't cancel
@@ -312,11 +373,6 @@ void ClientSideDetectionHost::DidNavigateMainFrame(
   if (!csd_service_) {
     return;
   }
-
-  // Cancel any pending classification request.
-  if (classification_request_.get()) {
-    classification_request_->Cancel();
-  }
   browse_info_.reset(new BrowseInfo);
 
   // Store redirect chain information.
@@ -324,14 +380,25 @@ void ClientSideDetectionHost::DidNavigateMainFrame(
     cur_host_ = params.url.host();
     cur_host_redirects_ = params.redirects;
   }
+  browse_info_->url = params.url;
   browse_info_->host_redirects = cur_host_redirects_;
   browse_info_->url_redirects = params.redirects;
   browse_info_->referrer = params.referrer.url;
   browse_info_->http_status_code = details.http_status_code;
+  browse_info_->page_id = params.page_id;
 
-  // Notify the renderer if it should classify this URL.
+  should_extract_malware_features_ = true;
+  should_classify_for_malware_ = false;
+  onload_complete_ = false;
+
+  // Check whether we can cassify the current URL for phishing or malware.
   classification_request_ = new ShouldClassifyUrlRequest(
-      params, web_contents(), csd_service_, database_manager_.get(), this);
+      params,
+      base::Bind(&ClientSideDetectionHost::OnPhishingPreClassificationDone,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&ClientSideDetectionHost::OnMalwarePreClassificationDone,
+                 weak_factory_.GetWeakPtr()),
+      web_contents(), csd_service_, database_manager_.get(), this);
   classification_request_->Start();
 }
 
@@ -414,6 +481,70 @@ void ClientSideDetectionHost::WebContentsDestroyed(WebContents* tab) {
   feature_extractor_.reset();
 }
 
+void ClientSideDetectionHost::OnPhishingPreClassificationDone(
+    bool should_classify) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (browse_info_.get() && should_classify) {
+    VLOG(1) << "Instruct renderer to start phishing detection for URL: "
+            << browse_info_->url;
+    content::RenderViewHost* rvh = web_contents()->GetRenderViewHost();
+    rvh->Send(new SafeBrowsingMsg_StartPhishingDetection(
+        rvh->GetRoutingID(), browse_info_->url));
+  }
+}
+
+void ClientSideDetectionHost::OnMalwarePreClassificationDone(
+    bool should_classify) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // If classification checks failed we should stop extracting malware features.
+  DVLOG(2) << "Malware pre-classification checks done. Should classify: "
+           << should_classify;
+  should_extract_malware_features_ = should_classify;
+  should_classify_for_malware_ = should_classify;
+  MaybeStartMalwareFeatureExtraction();
+}
+
+void ClientSideDetectionHost::DocumentOnLoadCompletedInMainFrame(
+    int32 page_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (!csd_service_ || !browse_info_.get())
+    return;
+  DVLOG(2) << "Main frame onload hander called.";
+  if (browse_info_->page_id != page_id) {
+    // Something weird is happening here.  The BrowseInfo page ID
+    // should always be the same as the most recent load.
+    UMA_HISTOGRAM_COUNTS("SBClientMalware.UnexpectedPageId", 1);

[Alexei Svitkine (slow), 2014/03/25 13:51:39]

Are you missing a histograms.xml entry for this one?

[noé, 2014/03/25 15:54:25]

Done.

+    return;
+  }
+  onload_complete_ = true;
+  MaybeStartMalwareFeatureExtraction();
+}
+
+void ClientSideDetectionHost::MaybeStartMalwareFeatureExtraction() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (csd_service_ && browse_info_.get() &&
+      should_classify_for_malware_ &&
+      onload_complete_) {
+    scoped_ptr<ClientMalwareRequest> malware_request(
+        new ClientMalwareRequest);
+    // Start browser-side malware feature extraction.  Once we're done it will
+    // send the malware client verdict request.
+    malware_request->set_url(browse_info_->url.spec());
+    const GURL& referrer = browse_info_->referrer;
+    if (referrer.SchemeIs("http")) {  // Only send http urls.
+      malware_request->set_referrer_url(referrer.spec());
+    }
+    // This function doesn't expect browse_info_ to stay around after this
+    // function returns.
+    feature_extractor_->ExtractMalwareFeatures(
+        browse_info_.get(),
+        malware_request.release(),
+        base::Bind(&ClientSideDetectionHost::MalwareFeatureExtractionDone,
+                   weak_factory_.GetWeakPtr()));
+    should_classify_for_malware_ = false;
+  }
+}
+
 void ClientSideDetectionHost::OnPhishingDetectionDone(
     const std::string& verdict_str) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -421,40 +552,15 @@ void ClientSideDetectionHost::OnPhishingDetectionDone(
   // this method is called.  The renderer should not start phishing detection
   // if there isn't any service class in the browser.
   DCHECK(csd_service_);
-  // There shouldn't be any pending requests because we revoke them everytime
-  // we navigate away.
-  DCHECK(!weak_factory_.HasWeakPtrs());
   DCHECK(browse_info_.get());
 
   // We parse the protocol buffer here.  If we're unable to parse it we won't
   // send the verdict further.
   scoped_ptr<ClientPhishingRequest> verdict(new ClientPhishingRequest);
   if (csd_service_ &&
-      !weak_factory_.HasWeakPtrs() &&
       browse_info_.get() &&
       verdict->ParseFromString(verdict_str) &&
       verdict->IsInitialized()) {
-    // We do the malware IP matching and request sending if the feature
-    // is enabled.
-    if (malware_report_enabled_ && !MalwareKillSwitchIsOn()) {
-      scoped_ptr<ClientMalwareRequest> malware_verdict(
-          new ClientMalwareRequest);
-      // Start browser-side malware feature extraction.  Once we're done it will
-      // send the malware client verdict request.
-      malware_verdict->set_url(verdict->url());
-      const GURL& referrer = browse_info_->referrer;
-      if (referrer.SchemeIs("http")) {  // Only send http urls.
-          malware_verdict->set_referrer_url(referrer.spec());
-      }
-      // This function doesn't expect browse_info_ to stay around after this
-      // function returns.
-      feature_extractor_->ExtractMalwareFeatures(
-          browse_info_.get(),
-          malware_verdict.release(),
-          base::Bind(&ClientSideDetectionHost::MalwareFeatureExtractionDone,
-                     weak_factory_.GetWeakPtr()));
-    }
-
     // We only send phishing verdict to the server if the verdict is phishing or
     // if a SafeBrowsing interstitial was already shown for this site.  E.g., a
     // malware or phishing interstitial was shown but the user clicked
@@ -472,14 +578,13 @@ void ClientSideDetectionHost::OnPhishingDetectionDone(
                      weak_factory_.GetWeakPtr()));
     }
   }
-  browse_info_.reset();
 }
 
 void ClientSideDetectionHost::MaybeShowPhishingWarning(GURL phishing_url,
                                                        bool is_phishing) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(2) << "Received server phishing verdict for URL:" << phishing_url
-          << " is_phishing:" << is_phishing;
+  DVLOG(2) << "Received server phishing verdict for URL:" << phishing_url
+           << " is_phishing:" << is_phishing;
   if (is_phishing) {
     DCHECK(web_contents());
     if (ui_manager_.get()) {
@@ -509,8 +614,8 @@ void ClientSideDetectionHost::MaybeShowMalwareWarning(GURL original_url,
                                                       GURL malware_url,
                                                       bool is_malware) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(2) << "Received server malawre IP verdict for URL:" << malware_url
-          << " is_malware:" << is_malware;
+  DVLOG(2) << "Received server malawre IP verdict for URL:" << malware_url
+           << " is_malware:" << is_malware;
   if (is_malware && malware_url.is_valid() && original_url.is_valid()) {
     DCHECK(web_contents());
     if (ui_manager_.get()) {
@@ -540,8 +645,8 @@ void ClientSideDetectionHost::FeatureExtractionDone(
     bool success,
     ClientPhishingRequest* request) {
   DCHECK(request);
-  VLOG(2) << "Feature extraction done (success:" << success << ") for URL: "
-          << request->url() << ". Start sending client phishing request.";
+  DVLOG(2) << "Feature extraction done (success:" << success << ") for URL: "
+           << request->url() << ". Start sending client phishing request.";
   ClientSideDetectionService::ClientReportPhishingRequestCallback callback;
   // If the client-side verdict isn't phishing we don't care about the server
   // response because we aren't going to display a warning.
@@ -559,8 +664,8 @@ void ClientSideDetectionHost::MalwareFeatureExtractionDone(
     bool feature_extraction_success,
     scoped_ptr<ClientMalwareRequest> request) {
   DCHECK(request.get());
-  VLOG(2) << "Malware Feature extraction done for URL: " << request->url()
-          << ", with badip url count:" << request->bad_ip_url_info_size();
+  DVLOG(2) << "Malware Feature extraction done for URL: " << request->url()
+           << ", with badip url count:" << request->bad_ip_url_info_size();
 
   // Send ping if there is matching features.
   if (feature_extraction_success && request->bad_ip_url_info_size() > 0) {
@@ -601,15 +706,13 @@ void ClientSideDetectionHost::Observe(
   DCHECK_EQ(type, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED);
   const ResourceRequestDetails* req = content::Details<ResourceRequestDetails>(
       details).ptr();
-  if (req && browse_info_.get() && malware_report_enabled_ &&
-      !MalwareKillSwitchIsOn()) {
-    if (req->url.is_valid()) {
-      UpdateIPUrlMap(req->socket_address.host() /* ip */,
-                     req->url.spec()  /* url */,
-                     req->method,
-                     req->referrer,
-                     req->resource_type);
-    }
+  if (req && browse_info_.get() &&
+      should_extract_malware_features_ && req->url.is_valid()) {
+    UpdateIPUrlMap(req->socket_address.host() /* ip */,
+                   req->url.spec()  /* url */,
+                   req->method,
+                   req->referrer,
+                   req->resource_type);
   }
 }
 
@@ -640,14 +743,4 @@ void ClientSideDetectionHost::set_safe_browsing_managers(
   database_manager_ = database_manager;
 }
 
-bool ClientSideDetectionHost::MalwareKillSwitchIsOn() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  return malware_killswitch_on_;
-}
-
-void ClientSideDetectionHost::SetMalwareKillSwitch(bool killswitch_on) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  malware_killswitch_on_ = killswitch_on;
-}
-
 }  // namespace safe_browsing