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

chrome/browser/safe_browsing/client_side_detection_host.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/client_side_detection_host.h"
 
 #include <vector>
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
@@ skipping 32 matching lines @@
 using content::ResourceRequestDetails;
 using content::WebContents;
 
 namespace safe_browsing {
 
 const int ClientSideDetectionHost::kMaxUrlsPerIP = 20;
 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_);
     DCHECK(database_manager_.get());
     DCHECK(host_);
   }
 
   void Start() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
     // 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);

[mattm, 2014/02/21 00:35:29]

Don't forget to update histograms.xml (it's in the same repo now so you don't
need a separate CL)

[noé, 2014/02/21 19:04:16]

Done.

 
     // Only classify [X]HTML documents.
     if (params_.contents_mime_type != "text/html" &&
-        params_.contents_mime_type != "application/xhtml+xml") {
+        params_.contents_mime_type != "application/xhtml+xml" &&
+        MaybeClassifyForPhishing()) {

[mattm, 2014/02/21 00:35:29]

It is confusing that some of the tests like this one check the
MaybeClassifyForX() as part of the condition, whereas others just depend on the
DontClassifyForX doing the check internally.

[noé, 2014/02/21 19:04:16]

That's a leftover.  We shouldn't need to check Maybe* anywhere except to know
whether we should continue at all.  For example, right before we post a new task
to the IO thread.

       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);
     }
 
-    // Don't run the phishing classifier if the tab is incognito.
+    // Don't run any classifier if the tab is incognito.
     if (web_contents_->GetBrowserContext()->IsOffTheRecord()) {
-      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);
-
-      return;
+      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 phishing start 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 (MaybeClassifyForPhishing() || MaybeClassifyForMalware()) {
+      BrowserThread::PostTask(
+          BrowserThread::IO,
+          FROM_HERE,
+          base::Bind(&ShouldClassifyUrlRequest::CheckCsdWhitelist,
+                     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().
     web_contents_ = NULL;
     csd_service_ = NULL;
     host_ = NULL;
   }
 
  private:
   friend class base::RefCountedThreadSafe<
       ClientSideDetectionHost::ShouldClassifyUrlRequest>;
 
   // Enum used to keep stats about why the pre-classification check failed.
   enum PreClassificationCheckFailures {
     OBSOLETE_NO_CLASSIFY_PROXY_FETCH,
     NO_CLASSIFY_PRIVATE_IP,
     NO_CLASSIFY_OFF_THE_RECORD,
     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_MAX  // Always add new values before this one.
   };
 
   // The destructor can be called either from the UI or the IO thread.
   virtual ~ShouldClassifyUrlRequest() { }
 
+  bool MaybeClassifyForPhishing() const {
+    return !start_phishing_classification_cb_.is_null();
+  }
+
+  bool MaybeClassifyForMalware() const {
+    return !start_malware_classification_cb_.is_null();
+  }
+
+  void DontClassifyForPhishing(PreClassificationCheckFailures reason) {
+    if (MaybeClassifyForPhishing()) {
+      // Track the first reason why we stopped classifying for phishing.
+      UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
+                                reason, NO_CLASSIFY_MAX);
+      start_phishing_classification_cb_.Run(false);
+    }
+    start_phishing_classification_cb_.Reset();
+  }
+
+  void DontClassifyForMalware(PreClassificationCheckFailures reason) {
+    if (MaybeClassifyForMalware()) {
+      // Track the first reason why we stopped classifying for phishing.

[mattm, 2014/02/21 00:35:29]

s/phishing/malware/

[noé, 2014/02/21 19:04:16]

Done.

+      UMA_HISTOGRAM_ENUMERATION("SBClientMalware.PreClassificationCheckFail",
+                                reason, NO_CLASSIFY_MAX);
+      start_malware_classification_cb_.Run(false);
+    }
+    start_malware_classification_cb_.Reset();
+  }
+
   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";
-      UMA_HISTOGRAM_ENUMERATION("SBClientPhishing.PreClassificationCheckFail",
-                                NO_CLASSIFY_MATCH_CSD_WHITELIST,
-                                NO_CLASSIFY_MAX);
+    // 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;
       return;
+    } 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;
+      }
     }
-
-    bool malware_killswitch_on = database_manager_->IsMalwareKillSwitchOn();
-
     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 (!MaybeClassifyForMalware() && !MaybeClassifyForPhishing()) {
+      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.
     if (csd_service_->IsInCache(params_.url)) {
       VLOG(1) << "Reporting limit skipped for " << params_.url
               << " as it was in the cache.";
       UMA_HISTOGRAM_COUNTS("SBClientPhishing.ReportLimitSkipped", 1);
     } 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 (MaybeClassifyForPhishing())
+      start_phishing_classification_cb_.Run(true);
+    if (MaybeClassifyForMalware())
+      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_;
   // We keep a ref pointer here just to make sure the safe browsing
   // database manager stays alive long enough.
   scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
   ClientSideDetectionHost* host_;
 
+  ShouldClassifyUrlCallback start_phishing_classification_cb_;
+  ShouldClassifyUrlCallback start_malware_classification_cb_;
+
   DISALLOW_COPY_AND_ASSIGN(ShouldClassifyUrlRequest);
 };
 
 // static
 ClientSideDetectionHost* ClientSideDetectionHost::Create(
     WebContents* tab) {
   return new ClientSideDetectionHost(tab);
 }
 
 ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
     : content::WebContentsObserver(tab),
       csd_service_(NULL),
+      should_extract_malware_features_(true),
+      should_classify_for_malware_(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();
   feature_extractor_.reset(new BrowserFeatureExtractor(tab, this));
   registrar_.Add(this, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED,
                  content::Source<WebContents>(tab));
 
   scoped_refptr<SafeBrowsingService> sb_service =
       g_browser_process->safe_browsing_service();
   if (sb_service.get()) {
     ui_manager_ = sb_service->ui_manager();
     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() {
   if (ui_manager_.get())
     ui_manager_->RemoveObserver(this);
 }
 
 bool ClientSideDetectionHost::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ClientSideDetectionHost, message)
@@ skipping 31 matching lines @@
   if (classification_request_.get()) {
     classification_request_->Cancel();
   }
   browse_info_.reset(new BrowseInfo);
 
   // Store redirect chain information.
   if (params.url.host() != cur_host_) {
     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.
+  // 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();
 }
 
 void ClientSideDetectionHost::OnSafeBrowsingHit(
     const SafeBrowsingUIManager::UnsafeResource& resource) {
   if (!web_contents() || !web_contents()->GetController().GetActiveEntry())
     return;
 
   // Check that the hit is either malware or phishing.
   if (resource.threat_type != SB_THREAT_TYPE_URL_PHISHING &&
@@ skipping 62 matching lines @@
 void ClientSideDetectionHost::WebContentsDestroyed(WebContents* tab) {
   DCHECK(tab);
   // Tell any pending classification request that it is being canceled.
   if (classification_request_.get()) {
     classification_request_->Cancel();
   }
   // Cancel all pending feature extractions.
   feature_extractor_.reset();
 }
 
+void ClientSideDetectionHost::OnPhishingPreClassificationDone(
+    bool should_classify) {
+  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) {
+  // If classification checks failed we should stop extracting malware features.
+  should_extract_malware_features_ = should_classify;
+  should_classify_for_malware_ = should_classify;

[mattm, 2014/02/21 00:35:29]

I think there are going to be some races between this callback,
ClientSideDetectionHost::Observe, and DocumentOnLoadCompletedInMainFrame.

[noé, 2014/02/21 19:04:16]

Good point.  Created a separate method with a member variable that keeps the
state around.

+}
+
+void ClientSideDetectionHost::DocumentOnLoadCompletedInMainFrame(
+    int32 page_id) {

[mattm, 2014/02/21 00:35:29]

Add thread dcheck (probably in more other methods too)

[noé, 2014/02/21 19:04:16]

Done.

+  if (!browse_info_.get()) {
+    // This only happens if OnPhishingDetectionDone is called before the
+    // document is done loading.  Phishing detection takes longer than
+    // that except when there is an error.

[mattm, 2014/02/21 00:35:29]

Not super clear, but it sounds like if there are slow loading resources or the
page's onload handler takes an arbitrarily long amount of time, this could
happen?  Seems like it would be better to just only clear the browse_info when
both are done with it.

[noé, 2014/02/21 19:04:16]

Removed that part.

Is there a help class to do reference counting by hand?  I'd rather avoid yet
another flag or member variable to track that both feature extractions are done.
 Any thoughts?  Right now, I decided not to delete the browse info.  It's a
pretty small object and it will be deleted once a new page loads.

[mattm, 2014/02/22 02:49:03]

Don't think there's too much like that. Could make it a RefCounted object and
keep separate refptrs for the malware and phishing.

I guess there is the RefCountedData wrapper but I'm not sure that is much better
than just making it RefCounted (and RefCountedData is thread-safe which isn't
necessary here.)

[noé, 2014/03/14 22:21:32]

RefCounted isn't a good fit here.  The object is a member of the host and should
be deleted once both malware and phishing classification is done.  Since I can't
manually increment the reference counter it's not that helpful.  I think the
object is small enough to keep it around.

+    UMA_HISTOGRAM_COUNTS("SBClientMalware.MissingBrowseInfo", 1);
+    return;
+  }
+  if (browse_info_->page_id == page_id &&

[mattm, 2014/02/21 00:35:29]

Is there an expected case where the pageids don't match? Should there be a
dcheck or uma log ?

[noé, 2014/02/21 19:04:16]

Added a UMA stat for that instead.

+      should_classify_for_malware_) {
+    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(browse_info_->url.spec());
+    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()));
+  }
+}
+
 void ClientSideDetectionHost::OnPhishingDetectionDone(
     const std::string& verdict_str) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // There is something seriously wrong if there is no service class but
   // 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
     // through.
     if (verdict->is_phishing() || DidShowSBInterstitial()) {
       if (DidShowSBInterstitial()) {
         browse_info_->unsafe_resource.reset(unsafe_resource_.release());
       }
       // Start browser-side feature extraction.  Once we're done it will send
       // the client verdict request.
       feature_extractor_->ExtractFeatures(
           browse_info_.get(),
           verdict.release(),
           base::Bind(&ClientSideDetectionHost::FeatureExtractionDone,
                      weak_factory_.GetWeakPtr()));
     }
   }
+  // It's safe to delete the browse info here because we're not going to further
+  // need it.  This method is typically called *after*
+  // DocumentOnLoadCompletedInMainFrame which is the last moment the malware
+  // classifier needs access to the browse info.
   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;
   if (is_phishing) {
     DCHECK(web_contents());
@@ skipping 109 matching lines @@
 }
 
 void ClientSideDetectionHost::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   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);
   }
 }
 
 bool ClientSideDetectionHost::DidShowSBInterstitial() const {
   if (unsafe_unique_page_id_ <= 0 || !web_contents()) {
     return false;
   }
   const NavigationEntry* nav_entry =
       web_contents()->GetController().GetActiveEntry();
   return (nav_entry && nav_entry->GetUniqueID() == unsafe_unique_page_id_);
@@ skipping 10 matching lines @@
   if (ui_manager_.get())
     ui_manager_->RemoveObserver(this);
 
   ui_manager_ = ui_manager;
   if (ui_manager)
     ui_manager_->AddObserver(this);
 
   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