Add a killswitch for CSD malware IP match and report feature. Use a new killswitch whitelist URL wh…

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 158 matching lines @@
         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);
       return;
     }
 
+    malware_killswitch_on_ = database_manager_->MalwareKillSwitchOn();
+
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&ShouldClassifyUrlRequest::CheckCache, this));
   }
 
   void CheckCache() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     if (canceled_) {
       return;
@@ skipping 53 matching lines @@
 // static
 ClientSideDetectionHost* ClientSideDetectionHost::Create(
     WebContents* tab) {
   return new ClientSideDetectionHost(tab);
 }
 
 ClientSideDetectionHost::ClientSideDetectionHost(WebContents* tab)
     : content::WebContentsObserver(tab),
       csd_service_(NULL),
       weak_factory_(this),
-      unsafe_unique_page_id_(-1) {
+      unsafe_unique_page_id_(-1),
+      malware_killswitch_on_(false) {
   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, csd_service_));
   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) {
@@ skipping 110 matching lines @@
   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()) {
-    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());
-    feature_extractor_->ExtractMalwareFeatures(
-        browse_info_.get(),
-        malware_verdict.get());
-    MalwareFeatureExtractionDone(malware_verdict.Pass());
+    // We do the malware IP matching and request sending if the feature
+    // is enabled
+    if (!malware_killswitch_on_) {

[mattm, 2013/05/13 20:39:25]

why not just do the database_manager_->MalwareKillSwitchOn() call here?

[kewang, 2013/05/14 08:21:41]

I saw the above code where it access the database_manager is on IO thread, while
this function is on UI thread. Seems some methods in database_manager not
require IO thread; putting the function call here is definitely better then.

BTW, what's the rule of requiring a function to be on IO or UI thread in
general?

[mattm, 2013/05/15 07:12:37]

Sorry, I think my above comment was incorrect, this probably should only be
called on the IO thread.
In general you need to check the whatever members the method accesses, see what
threads those members are set&read on, and whether they have locking protecting
them or not.  In the case of afeBrowsingDatabaseNew::MalwareIPMatchKillSwitchOn
it does have locking so I initially thought it would be ok to call this on any
thread, but looking at the other methods it needs to go through to get to there
it doesn't look safe actually. (SafeBrowsingDatabaseManager::MalwareKillSwitchOn
specifically, the database_ member looks like it's intended to be used on the IO
thread)

[kewang, 2013/05/15 13:45:07]

So I kept the code to check switch from database_manager_ at the
CheckCsdWhitelist() in IO thread, set the variable in host_, and do check here.
This looks ok?

+      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());
+      feature_extractor_->ExtractMalwareFeatures(
+          browse_info_.get(), malware_verdict.get());
+      MalwareFeatureExtractionDone(malware_verdict.Pass());
+    }
 
     // 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
@@ skipping 129 matching lines @@
     ui_manager_->RemoveObserver(this);
 
   ui_manager_ = ui_manager;
   if (ui_manager)
     ui_manager_->AddObserver(this);
 
   database_manager_ = database_manager;
 }
 
 }  // namespace safe_browsing