SafeBrowsing: Move enabled_ to base class DB Manager

chrome/browser/safe_browsing/local_database_manager.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/local_database_manager.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
@@ skipping 253 matching lines @@
     }
   } else {
     NOTREACHED();
   }
 }
 
 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),
       enable_extension_blacklist_(false),
       enable_ip_blacklist_(false),
       enable_unwanted_software_blacklist_(true),
       enable_module_whitelist_(true),
       update_in_progress_(false),
       database_update_in_progress_(false),
       closing_database_(false),
@@ skipping 378 matching lines @@
   DCHECK(enabled_);
   safe_browsing_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&LocalSafeBrowsingDatabaseManager::OnResetDatabase, this));
 }
 
 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;
 
   // Only get a new task runner if there isn't one already. If the service has
   // previously been started and stopped, a task runner could already exist.
   if (!safe_browsing_task_runner_) {
     base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
     safe_browsing_task_runner_ =
         pool->GetSequencedTaskRunnerWithShutdownBehavior(
             pool->GetSequenceToken(),
             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(
     bool update_succeeded) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE,
       content::Source<SafeBrowsingDatabaseManager>(this),
       content::Details<bool>(&update_succeeded));
 }
@@ skipping 14 matching lines @@
     const QueuedCheck& other) = default;
 
 LocalSafeBrowsingDatabaseManager::QueuedCheck::~QueuedCheck() {}
 
 void LocalSafeBrowsingDatabaseManager::DoStopOnIOThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   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();
     if (queued.client) {
       SafeBrowsingCheck sb_check(std::vector<GURL>(1, queued.url),
                                  std::vector<SBFullHash>(), queued.client,
                                  queued.check_type, queued.expected_threats);
       sb_check.OnSafeBrowsingResult();
     }
     queued_checks_.pop_front();
@@ skipping 495 matching lines @@
       base::Bind(&LocalSafeBrowsingDatabaseManager::TimeoutCallback,
                  check_ptr->weak_ptr_factory_->GetWeakPtr(), check_ptr),
       check_timeout_);
 }
 
 bool LocalSafeBrowsingDatabaseManager::IsDownloadProtectionEnabled() const {
   return enable_download_protection_;
 }
 
 }  // namespace safe_browsing