SafeBrowsing: Move enabled_ to base class DB Manager

chrome/browser/safe_browsing/local_database_manager.cc

Index: chrome/browser/safe_browsing/local_database_manager.cc
diff --git a/chrome/browser/safe_browsing/local_database_manager.cc b/chrome/browser/safe_browsing/local_database_manager.cc
index 615a7b60dfb8bc2bedc2e6d02f8ff1d20ad5fb1e..d8bd5fa5909fb1320f6821a7c6d4e2aae34e8eec 100644
--- a/chrome/browser/safe_browsing/local_database_manager.cc
+++ b/chrome/browser/safe_browsing/local_database_manager.cc
@@ -271,7 +271,6 @@ LocalSafeBrowsingDatabaseManager::LocalSafeBrowsingDatabaseManager(
     const scoped_refptr<SafeBrowsingService>& service)
     : sb_service_(service),
       database_(NULL),
-      enabled_(false),
       enable_download_protection_(false),
       enable_csd_whitelist_(false),
       enable_download_whitelist_(false),
@@ -670,7 +669,6 @@ void LocalSafeBrowsingDatabaseManager::StartOnIOThread(
     net::URLRequestContextGetter* request_context_getter,
     const V4ProtocolConfig& config) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  SafeBrowsingDatabaseManager::StartOnIOThread(request_context_getter, config);
 
   if (enabled_)
     return;
@@ -685,19 +683,22 @@ void LocalSafeBrowsingDatabaseManager::StartOnIOThread(
             base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   }
 
-  enabled_ = true;
+  // Call this after the enabled_ check. It will set enabled_ to true.
+  SafeBrowsingDatabaseManager::StartOnIOThread(request_context_getter, config);
 
   MakeDatabaseAvailable();
 }
 
 void LocalSafeBrowsingDatabaseManager::StopOnIOThread(bool shutdown) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  SafeBrowsingDatabaseManager::StopOnIOThread(shutdown);
 
   DoStopOnIOThread();
   if (shutdown) {
     sb_service_ = NULL;
   }
+
+  // Call this last. It will set enabled_ to false.
+  SafeBrowsingDatabaseManager::StopOnIOThread(shutdown);

[Nathan Parker, 2017/01/23 23:32:34]

I think we should set enabled_=false at the top of this function, since the
client-callback calls in DoStopOnIOThread() and in SBDM::StopOnIOThread() could
call back into the DB Mgr, and they should see that it's disabled..

And it's probably safer to have the symmetry of calling the base class's Start
function before the sub-class's code, and calling the base class's Stop after
the sub-class's code (like how members get initialized).  That would require
splitting the enable=* code out from SBDM::Start/StopOnIOThread().

(You could have a base class SetEnabled(bool) to enforce that it's manipulated
on the IO thread.  That doesn't really add much through.)

[kcarattini, 2017/01/24 03:21:23]

Done (I think). Now only the subclasses set enabled_ and base class calls are
int he order you specified. PTAL.

 }
 
 void LocalSafeBrowsingDatabaseManager::NotifyDatabaseUpdateFinished(
@@ -732,8 +733,6 @@ void LocalSafeBrowsingDatabaseManager::DoStopOnIOThread() {
   if (!enabled_)
     return;
 
-  enabled_ = false;
-
   // Delete queued checks, calling back any clients with 'SB_THREAT_TYPE_SAFE'.
   while (!queued_checks_.empty()) {
     QueuedCheck queued = queued_checks_.front();