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

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 d9107dd4aaaee54a1dcc06927389814ab711b14c..9a17adc660f576a92349c51014088394a637e359 100644
--- a/chrome/browser/safe_browsing/client_side_detection_host.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_host.cc
@@ -176,6 +176,8 @@ class ClientSideDetectionHost::ShouldClassifyUrlRequest
       return;
     }
 
+    malware_killswitch_on_ = database_manager_->MalwareKillSwitchOn();
+
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
@@ -249,7 +251,8 @@ 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();
@@ -380,14 +383,18 @@ void ClientSideDetectionHost::OnPhishingDetectionDone(
       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