Replace deprecated base::NonThreadSafe in extensions in favor of SequenceChecker.
Note to crash team: This CL is a refactor and has no intended behavior change.
This change was scripted by https://crbug.com/676387#c8.
Note-worthy for the reviewer:
* SequenceChecker enforces thread-safety but not thread-affinity!
If the classes that were updated are thread-affine (use thread local
storage or a third-party API that does) they should be migrated to
ThreadChecker instead.
* ~NonThreadSafe() used to implicitly check in its destructor
~Sequence/ThreadChecker() doesn't by design. To keep this CL a
no-op, an explicit check was added to the destructor of migrated
classes.
* NonThreadSafe used to provide access to subclasses, as such
the |sequence_checker_| member was made protected rather than
private where necessary.
BUG=676387
This CL was uploaded by git cl split.
[email protected]
Review-Url: https://codereview.chromium.org/2915523002
Cr-Commit-Position: refs/heads/master@{#476280}
diff --git a/extensions/browser/api/api_resource_manager.h b/extensions/browser/api/api_resource_manager.h
index 064e7454..8626e21 100644
--- a/extensions/browser/api/api_resource_manager.h
+++ b/extensions/browser/api/api_resource_manager.h
@@ -14,7 +14,6 @@
#include "base/scoped_observer.h"
#include "base/sequence_checker.h"
#include "base/sequenced_task_runner.h"
-#include "base/threading/non_thread_safe.h"
#include "base/threading/sequenced_worker_pool.h"
#include "components/keyed_service/core/keyed_service.h"
#include "content/public/browser/browser_thread.h"
@@ -89,7 +88,6 @@
// }
template <class T, typename ThreadingTraits = NamedThreadTraits<T>>
class ApiResourceManager : public BrowserContextKeyedAPI,
- public base::NonThreadSafe,
public ExtensionRegistryObserver,
public ProcessManagerObserver {
public:
@@ -102,7 +100,7 @@
}
virtual ~ApiResourceManager() {
- DCHECK(CalledOnValidThread());
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(ThreadingTraits::IsMessageLoopValid())
<< "A unit test is using an ApiResourceManager but didn't provide "
"the thread message loop needed for that kind of resource. "
@@ -365,6 +363,8 @@
extension_registry_observer_;
ScopedObserver<ProcessManager, ProcessManagerObserver>
process_manager_observer_;
+
+ SEQUENCE_CHECKER(sequence_checker_);
};
template <class T>
diff --git a/extensions/browser/extension_throttle_manager.cc b/extensions/browser/extension_throttle_manager.cc
index c33d0207..bc3a810a 100644
--- a/extensions/browser/extension_throttle_manager.cc
+++ b/extensions/browser/extension_throttle_manager.cc
@@ -40,6 +40,7 @@
}
ExtensionThrottleManager::~ExtensionThrottleManager() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
@@ -69,7 +70,9 @@
scoped_refptr<ExtensionThrottleEntryInterface>
ExtensionThrottleManager::RegisterRequestUrl(const GURL& url) {
- DCHECK(!enable_thread_checks_ || CalledOnValidThread());
+#if DCHECK_IS_ON()
+ DCHECK(!enable_thread_checks_ || sequence_checker_.CalledOnValidSequence());
+#endif // DCHECK_IS_ON()
// Normalize the url.
std::string url_id = GetIdFromUrl(url);
diff --git a/extensions/browser/extension_throttle_manager.h b/extensions/browser/extension_throttle_manager.h
index cad766b..6b4723a 100644
--- a/extensions/browser/extension_throttle_manager.h
+++ b/extensions/browser/extension_throttle_manager.h
@@ -11,7 +11,7 @@
#include "base/macros.h"
#include "base/memory/ref_counted.h"
-#include "base/threading/non_thread_safe.h"
+#include "base/sequence_checker.h"
#include "base/threading/platform_thread.h"
#include "extensions/browser/extension_throttle_entry.h"
#include "net/base/backoff_entry.h"
@@ -40,8 +40,7 @@
// clean out outdated entries. URL ID consists of lowercased scheme, host, port
// and path. All URLs converted to the same ID will share the same entry.
class ExtensionThrottleManager
- : NON_EXPORTED_BASE(public base::NonThreadSafe),
- public net::NetworkChangeNotifier::IPAddressObserver,
+ : public net::NetworkChangeNotifier::IPAddressObserver,
public net::NetworkChangeNotifier::ConnectionTypeObserver {
public:
ExtensionThrottleManager();
@@ -178,6 +177,8 @@
// This is NULL when it is not set for tests.
std::unique_ptr<net::BackoffEntry::Policy> backoff_policy_for_tests_;
+ SEQUENCE_CHECKER(sequence_checker_);
+
DISALLOW_COPY_AND_ASSIGN(ExtensionThrottleManager);
};
diff --git a/extensions/browser/quota_service.cc b/extensions/browser/quota_service.cc
index 99967d7..3d53713 100644
--- a/extensions/browser/quota_service.cc
+++ b/extensions/browser/quota_service.cc
@@ -33,7 +33,7 @@
}
QuotaService::~QuotaService() {
- DCHECK(CalledOnValidThread());
+ DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
purge_timer_.Stop();
Purge();
}
@@ -42,7 +42,7 @@
ExtensionFunction* function,
const base::ListValue* args,
const base::TimeTicks& event_time) {
- DCHECK(CalledOnValidThread());
+ DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
if (function->ShouldSkipQuotaLimiting())
return std::string();
@@ -86,7 +86,7 @@
}
void QuotaService::Purge() {
- DCHECK(CalledOnValidThread());
+ DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
for (auto it = function_heuristics_.begin(); it != function_heuristics_.end();
function_heuristics_.erase(it++)) {
it->second.clear();
diff --git a/extensions/browser/quota_service.h b/extensions/browser/quota_service.h
index 433ea32..ef71f3c 100644
--- a/extensions/browser/quota_service.h
+++ b/extensions/browser/quota_service.h
@@ -24,7 +24,7 @@
#include "base/compiler_specific.h"
#include "base/containers/hash_tables.h"
#include "base/macros.h"
-#include "base/threading/non_thread_safe.h"
+#include "base/threading/thread_checker.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "base/values.h"
@@ -44,7 +44,7 @@
// called and destroyed on the same thread, due to its use of a RepeatingTimer.
// It is not a KeyedService because instances exist on both the UI
// and IO threads.
-class QuotaService : public base::NonThreadSafe {
+class QuotaService {
public:
// Some concrete heuristics (declared below) that ExtensionFunctions can
// use to help the service make decisions about quota violations.
@@ -91,6 +91,8 @@
// Each heuristic will be evaluated and ANDed together to get a final answer.
std::map<ExtensionId, FunctionHeuristicsMap> function_heuristics_;
+ THREAD_CHECKER(thread_checker_);
+
DISALLOW_COPY_AND_ASSIGN(QuotaService);
};
diff --git a/extensions/browser/value_store/value_store_frontend.cc b/extensions/browser/value_store/value_store_frontend.cc
index 964fdbe..cf2379a 100644
--- a/extensions/browser/value_store/value_store_frontend.cc
+++ b/extensions/browser/value_store/value_store_frontend.cc
@@ -109,15 +109,17 @@
ValueStoreFrontend::ValueStoreFrontend(
const scoped_refptr<ValueStoreFactory>& store_factory,
BackendType backend_type)
- : backend_(new Backend(store_factory, backend_type)) {}
+ : backend_(new Backend(store_factory, backend_type)) {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
ValueStoreFrontend::~ValueStoreFrontend() {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
}
void ValueStoreFrontend::Get(const std::string& key,
const ReadCallback& callback) {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
base::Bind(&ValueStoreFrontend::Backend::Get,
@@ -126,7 +128,7 @@
void ValueStoreFrontend::Set(const std::string& key,
std::unique_ptr<base::Value> value) {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
base::Bind(&ValueStoreFrontend::Backend::Set,
@@ -134,7 +136,7 @@
}
void ValueStoreFrontend::Remove(const std::string& key) {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
base::Bind(&ValueStoreFrontend::Backend::Remove,
diff --git a/extensions/browser/value_store/value_store_frontend.h b/extensions/browser/value_store/value_store_frontend.h
index 015c6f3..82d0a99 100644
--- a/extensions/browser/value_store/value_store_frontend.h
+++ b/extensions/browser/value_store/value_store_frontend.h
@@ -12,7 +12,6 @@
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
-#include "base/threading/non_thread_safe.h"
#include "base/values.h"
#include "extensions/browser/value_store/value_store.h"
@@ -21,9 +20,7 @@
} // namespace extensions
// A frontend for a LeveldbValueStore, for use on the UI thread.
-class ValueStoreFrontend
- : public base::SupportsWeakPtr<ValueStoreFrontend>,
- public base::NonThreadSafe {
+class ValueStoreFrontend : public base::SupportsWeakPtr<ValueStoreFrontend> {
public:
// The kind of extensions data stored in a backend.
enum class BackendType { RULES, STATE };
diff --git a/extensions/browser/warning_service.cc b/extensions/browser/warning_service.cc
index c6f643b..1f5cf12 100644
--- a/extensions/browser/warning_service.cc
+++ b/extensions/browser/warning_service.cc
@@ -16,14 +16,16 @@
WarningService::WarningService(content::BrowserContext* browser_context)
: browser_context_(browser_context), extension_registry_observer_(this) {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (browser_context_) {
extension_registry_observer_.Add(ExtensionRegistry::Get(
ExtensionsBrowserClient::Get()->GetOriginalContext(browser_context_)));
}
}
-WarningService::~WarningService() {}
+WarningService::~WarningService() {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
// static
WarningService* WarningService::Get(content::BrowserContext* browser_context) {
@@ -32,7 +34,7 @@
void WarningService::ClearWarnings(
const std::set<Warning::WarningType>& types) {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
ExtensionIdSet affected_extensions;
for (WarningSet::iterator i = warnings_.begin();
i != warnings_.end();) {
@@ -50,7 +52,7 @@
std::set<Warning::WarningType> WarningService::
GetWarningTypesAffectingExtension(const std::string& extension_id) const {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
std::set<Warning::WarningType> result;
for (WarningSet::const_iterator i = warnings_.begin();
i != warnings_.end(); ++i) {
@@ -62,7 +64,7 @@
std::vector<std::string> WarningService::GetWarningMessagesForExtension(
const std::string& extension_id) const {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
std::vector<std::string> result;
const ExtensionSet& extension_set =
@@ -77,7 +79,7 @@
}
void WarningService::AddWarnings(const WarningSet& warnings) {
- DCHECK(CalledOnValidThread());
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
ExtensionIdSet affected_extensions;
for (const Warning& warning : warnings) {
diff --git a/extensions/browser/warning_service.h b/extensions/browser/warning_service.h
index 8fd3b633..5accc0d 100644
--- a/extensions/browser/warning_service.h
+++ b/extensions/browser/warning_service.h
@@ -11,7 +11,6 @@
#include "base/observer_list.h"
#include "base/scoped_observer.h"
-#include "base/threading/non_thread_safe.h"
#include "components/keyed_service/core/keyed_service.h"
#include "extensions/browser/extension_registry_observer.h"
#include "extensions/browser/warning_set.h"
@@ -30,9 +29,7 @@
// conflicting modifications of network requests by extensions, slow extensions,
// etc.) trigger a warning badge in the UI and and provide means to resolve
// them. This class must be used on the UI thread only.
-class WarningService : public KeyedService,
- public ExtensionRegistryObserver,
- public base::NonThreadSafe {
+class WarningService : public KeyedService, public ExtensionRegistryObserver {
public:
class Observer {
public:
diff --git a/extensions/browser/warning_service_unittest.cc b/extensions/browser/warning_service_unittest.cc
index 83a3ea0..96b6c5d3 100644
--- a/extensions/browser/warning_service_unittest.cc
+++ b/extensions/browser/warning_service_unittest.cc
@@ -5,6 +5,7 @@
#include "extensions/browser/warning_service.h"
#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
#include "extensions/browser/extensions_test.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -45,6 +46,7 @@
// Check that inserting a warning triggers notifications, whereas inserting
// the same warning again is silent.
TEST_F(WarningServiceTest, SetWarning) {
+ content::TestBrowserThreadBundle thread_bundle_;
content::TestBrowserContext browser_context;
TestWarningService warning_service(&browser_context);
MockObserver observer;
@@ -68,6 +70,7 @@
// Check that ClearWarnings deletes exactly the specified warnings and
// triggers notifications where appropriate.
TEST_F(WarningServiceTest, ClearWarnings) {
+ content::TestBrowserThreadBundle thread_bundle_;
content::TestBrowserContext browser_context;
TestWarningService warning_service(&browser_context);
MockObserver observer;
