Add "enabled" state to the ClientSideDetectionService.

chrome/browser/safe_browsing/client_side_detection_service.cc

Index: chrome/browser/safe_browsing/client_side_detection_service.cc
diff --git a/chrome/browser/safe_browsing/client_side_detection_service.cc b/chrome/browser/safe_browsing/client_side_detection_service.cc
index fc3aa2c7d637203025e632e7f45777e2958b29f1..3ee6451c64db8a70cb136987cb159b76e995981b 100644
--- a/chrome/browser/safe_browsing/client_side_detection_service.cc
+++ b/chrome/browser/safe_browsing/client_side_detection_service.cc
@@ -63,7 +63,8 @@ ClientSideDetectionService::CacheState::CacheState(bool phish, base::Time time)
 
 ClientSideDetectionService::ClientSideDetectionService(
     net::URLRequestContextGetter* request_context_getter)
-    : ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
+    : enabled_(false),
+      ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
       request_context_getter_(request_context_getter) {
   registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED,
                  NotificationService::AllSources());
@@ -86,17 +87,31 @@ ClientSideDetectionService* ClientSideDetectionService::Create(
     UMA_HISTOGRAM_COUNTS("SBClientPhishing.InitPrivateNetworksFailed", 1);
     return NULL;
   }
-  // We fetch the model at every browser restart.  In a lot of cases the model
-  // will be in the cache so it won't actually be fetched from the network.
-  // We delay the first model fetch to avoid slowing down browser startup.
-  MessageLoop::current()->PostDelayedTask(
-      FROM_HERE,
-      service->method_factory_.NewRunnableMethod(
-          &ClientSideDetectionService::StartFetchModel),
-      kInitialClientModelFetchDelayMs);
   return service.release();
 }
 
+void ClientSideDetectionService::SetEnabled(bool enabled) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (enabled == enabled_)
+    return;
+  enabled_ = enabled;
+  if (enabled_) {
+    // Refresh the model when the service is enabled.  This can happen when the
+    // preference is toggled, or early during startup if the preference is
+    // already enabled. In a lot of cases the model will be in the cache so it
+    // won't actually be fetched from the network.
+    // We delay the first model fetch to avoid slowing down browser startup.
+    ScheduleFetchModel(kInitialClientModelFetchDelayMs);
+  } else {
+    // Cancel pending requests.
+    model_fetcher_.reset();
+    cache_.clear();
+    // Any pending |client_phishing_reports_| will eventually call back into
+    // |OnURLFetchComplete|. Those requests aren't cancelled here, because
+    // we have to invoke the callback.

[mattm, 2011/08/08 21:59:46]

Hm, are you sure this can't use the same clean-up mechanism the destructor uses?

[Joao da Silva, 2011/08/09 09:16:12]

It can, but deleting the URLFetchers will cancel their callbacks (to
OnURLFetchComplete), and therefore the callbacks passed to
SendClientReportPhishingRequest won't be called, and the client of CSDS might be
waiting for it.

We could also invoke all the callbacks with a "false" verdict before deleting
them here... wdyt?

[mattm, 2011/08/10 23:38:26]

With the current implementation it wouldn't care, but I guess it might be better
not to depend on that.
Yeah, that seems cleaner than waiting for the requests to complete and checking
the enabled flag in HandlePhishingVerdict.

[Joao da Silva, 2011/08/11 11:51:50]

Agree, done.

+  }
+}
+
 void ClientSideDetectionService::SendClientReportPhishingRequest(
     ClientPhishingRequest* verdict,
     ClientReportPhishingRequestCallback* callback) {
@@ -198,15 +213,25 @@ void ClientSideDetectionService::SendModelToRenderers() {
   }
 }
 
+void ClientSideDetectionService::ScheduleFetchModel(int64 delay_ms) {
+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      method_factory_.NewRunnableMethod(
+          &ClientSideDetectionService::StartFetchModel),
+      delay_ms);
+}
+
 void ClientSideDetectionService::StartFetchModel() {
-  // Start fetching the model either from the cache or possibly from the
-  // network if the model isn't in the cache.
-  model_fetcher_.reset(URLFetcher::Create(0 /* ID is not used */,
-                                          GURL(kClientModelUrl),
-                                          URLFetcher::GET,
-                                          this));
-  model_fetcher_->set_request_context(request_context_getter_.get());
-  model_fetcher_->Start();
+  if (enabled_) {
+    // Start fetching the model either from the cache or possibly from the
+    // network if the model isn't in the cache.
+    model_fetcher_.reset(URLFetcher::Create(0 /* ID is not used */,
+                                            GURL(kClientModelUrl),
+                                            URLFetcher::GET,
+                                            this));
+    model_fetcher_->set_request_context(request_context_getter_.get());
+    model_fetcher_->Start();
+  }
 }
 
 void ClientSideDetectionService::EndFetchModel(ClientModelStatus status) {
@@ -230,11 +255,7 @@ void ClientSideDetectionService::EndFetchModel(ClientModelStatus status) {
   model_max_age_.reset();
 
   // Schedule the next model reload.
-  MessageLoop::current()->PostDelayedTask(
-      FROM_HERE,
-      method_factory_.NewRunnableMethod(
-          &ClientSideDetectionService::StartFetchModel),
-      delay_ms);
+  ScheduleFetchModel(delay_ms);
 }
 
 void ClientSideDetectionService::StartClientReportPhishingRequest(
@@ -244,6 +265,12 @@ void ClientSideDetectionService::StartClientReportPhishingRequest(
   scoped_ptr<ClientPhishingRequest> request(verdict);
   scoped_ptr<ClientReportPhishingRequestCallback> cb(callback);
 
+  if (!enabled_) {
+    if (cb.get())
+      cb->Run(GURL(request->url()), false);
+    return;
+  }
+
   std::string request_data;
   if (!request->SerializeToString(&request_data)) {
     UMA_HISTOGRAM_COUNTS("SBClientPhishing.RequestNotSerialized", 1);
@@ -327,10 +354,14 @@ void ClientSideDetectionService::HandlePhishingVerdict(
   bool is_phishing = false;
   if (status.is_success() && RC_REQUEST_OK == response_code &&
       response.ParseFromString(data)) {
-    // Cache response, possibly flushing an old one.
-    cache_[info->phishing_url] =
-        make_linked_ptr(new CacheState(response.phishy(), base::Time::Now()));
-    is_phishing = response.phishy();
+    // The service could have been disabled while this request was in flight.
+    // In that case, ignore the response and just report not phishing.
+    if (enabled_) {
+      // Cache response, possibly flushing an old one.
+      cache_[info->phishing_url] =
+          make_linked_ptr(new CacheState(response.phishy(), base::Time::Now()));
+      is_phishing = response.phishy();
+    }
   } else {
     DLOG(ERROR) << "Unable to get the server verdict for URL: "
                 << info->phishing_url << " status: " << status.status() << " "