[TaskScheduler]: Add assertions for ScopedBlockingCall.

This CL adds assertions in ScopedBlockingCall.
To distinguish between blocking and sync primitive assertions, a new class
was added, ScopedBlockingCallWithBaseSyncPrimitives. Usages of
ScopedBlockingCall were replaced by ScopedBlockingCallWithBaseSyncPrimitives
when appropriate.
Corresponding assertions were added (AssertBlockingAllowed and AssertBaseSyncPrimitivesAllowed)
in both ScopedBlockingCall and ScopedBlockingCallWithBaseSyncPrimitives.
In the future, either ScopedBlockingCall or ScopedBlockingCallWithBaseSyncPrimitives
will be used to mark all blocking tasks.

Bug: 874080
Change-Id: I8277436d4dd25dedb47d6fcdacc45f3905397647
Reviewed-on: https://chromium-review.googlesource.com/1178326Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586297}

base/synchronization/condition_variable_posix.cc

@@ -63,8 +63,8 @@ ConditionVariable::~ConditionVariable() {
 }
 
 void ConditionVariable::Wait() {
-  internal::AssertBaseSyncPrimitivesAllowed();
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
 #if DCHECK_IS_ON()
   user_lock_->CheckHeldAndUnmark();
 #endif
@@ -76,8 +76,8 @@ void ConditionVariable::Wait() {
 }
 
 void ConditionVariable::TimedWait(const TimeDelta& max_time) {
-  internal::AssertBaseSyncPrimitivesAllowed();
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
   int64_t usecs = max_time.InMicroseconds();
   struct timespec relative_time;
   relative_time.tv_sec = usecs / Time::kMicrosecondsPerSecond;

base/synchronization/condition_variable_win.cc

@@ -30,8 +30,8 @@ void ConditionVariable::Wait() {
 }
 
 void ConditionVariable::TimedWait(const TimeDelta& max_time) {
-  internal::AssertBaseSyncPrimitivesAllowed();
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
   DWORD timeout = static_cast<DWORD>(max_time.InMilliseconds());
 
 #if DCHECK_IS_ON()

base/synchronization/waitable_event_mac.cc

@@ -111,8 +111,8 @@ bool WaitableEvent::TimedWait(const TimeDelta& wait_delta) {
 }
 
 bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
-  internal::AssertBaseSyncPrimitivesAllowed();
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
   // Record the event that this thread is blocking upon (for hang diagnosis).
   debug::ScopedEventWaitActivity event_activity(this);
 
@@ -170,9 +170,9 @@ bool WaitableEvent::UseSlowWatchList(ResetPolicy policy) {
 
 // static
 size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables, size_t count) {
-  internal::AssertBaseSyncPrimitivesAllowed();
   DCHECK(count) << "Cannot wait on no events";
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
   // Record an event (the first) that this thread is blocking upon.
   debug::ScopedEventWaitActivity event_activity(raw_waitables[0]);
 

base/synchronization/waitable_event_posix.cc

@@ -162,8 +162,8 @@ bool WaitableEvent::TimedWait(const TimeDelta& wait_delta) {
 }
 
 bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
-  internal::AssertBaseSyncPrimitivesAllowed();
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
   // Record the event that this thread is blocking upon (for hang diagnosis).
   base::debug::ScopedEventWaitActivity event_activity(this);
 
@@ -237,9 +237,9 @@ cmp_fst_addr(const std::pair<WaitableEvent*, unsigned> &a,
 // static
 size_t WaitableEvent::WaitMany(WaitableEvent** raw_waitables,
                                size_t count) {
-  internal::AssertBaseSyncPrimitivesAllowed();
   DCHECK(count) << "Cannot wait on no events";
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
   // Record an event (the first) that this thread is blocking upon.
   base::debug::ScopedEventWaitActivity event_activity(raw_waitables[0]);
 

base/synchronization/waitable_event_win.cc

@@ -53,8 +53,8 @@ bool WaitableEvent::IsSignaled() {
 }
 
 void WaitableEvent::Wait() {
-  internal::AssertBaseSyncPrimitivesAllowed();
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
   // Record the event that this thread is blocking upon (for hang diagnosis).
   base::debug::ScopedEventWaitActivity event_activity(this);
 
@@ -69,7 +69,8 @@ namespace {
 
 // Helper function called from TimedWait and TimedWaitUntil.
 bool WaitUntil(HANDLE handle, const TimeTicks& now, const TimeTicks& end_time) {
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
 
   TimeDelta delta = end_time - now;
   DCHECK_GT(delta, TimeDelta());
@@ -136,8 +137,8 @@ bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
 size_t WaitableEvent::WaitMany(WaitableEvent** events, size_t count) {
   DCHECK(count) << "Cannot wait on no events";
 
-  internal::AssertBaseSyncPrimitivesAllowed();
-  ScopedBlockingCall scoped_blocking_call(BlockingType::MAY_BLOCK);
+  internal::ScopedBlockingCallWithBaseSyncPrimitives scoped_blocking_call(
+      BlockingType::MAY_BLOCK);
   // Record an event (the first) that this thread is blocking upon.
   base::debug::ScopedEventWaitActivity event_activity(events[0]);
 

base/task/task_scheduler/scheduler_worker_pool_impl_unittest.cc

@@ -1000,8 +1000,8 @@ class TaskSchedulerWorkerPoolBlockingTest
 
   void SetUp() override {
     TaskSchedulerWorkerPoolImplTestBase::CommonSetUp();
-    task_runner_ =
-        worker_pool_->CreateTaskRunnerWithTraits({WithBaseSyncPrimitives()});
+    task_runner_ = worker_pool_->CreateTaskRunnerWithTraits(
+        {MayBlock(), WithBaseSyncPrimitives()});
   }
 
   void TearDown() override {
@@ -1268,8 +1268,8 @@ TEST_F(TaskSchedulerWorkerPoolBlockingTest, ThreadBlockUnblockPremature) {
 TEST_F(TaskSchedulerWorkerPoolBlockingTest,
        MayBlockIncreaseCapacityNestedWillBlock) {
   ASSERT_EQ(worker_pool_->GetMaxTasksForTesting(), kMaxTasks);
-  auto task_runner =
-      worker_pool_->CreateTaskRunnerWithTraits({WithBaseSyncPrimitives()});
+  auto task_runner = worker_pool_->CreateTaskRunnerWithTraits(
+      {MayBlock(), WithBaseSyncPrimitives()});
   WaitableEvent can_return;
 
   // Saturate the pool so that a MAY_BLOCK ScopedBlockingCall would increment
@@ -1333,7 +1333,8 @@ TEST(TaskSchedulerWorkerPoolOverCapacityTest, VerifyCleanup) {
       SchedulerWorkerPoolImpl::WorkerEnvironment::NONE);
 
   scoped_refptr<TaskRunner> task_runner =
-      worker_pool.CreateTaskRunnerWithTraits({WithBaseSyncPrimitives()});
+      worker_pool.CreateTaskRunnerWithTraits(
+          {MayBlock(), WithBaseSyncPrimitives()});
 
   WaitableEvent threads_running;
   WaitableEvent threads_continue;

base/threading/scoped_blocking_call.cc

@@ -6,6 +6,7 @@
 
 #include "base/lazy_instance.h"
 #include "base/threading/thread_local.h"
+#include "base/threading/thread_restrictions.h"
 
 namespace base {
 
@@ -15,12 +16,15 @@ LazyInstance<ThreadLocalPointer<internal::BlockingObserver>>::Leaky
     tls_blocking_observer = LAZY_INSTANCE_INITIALIZER;
 
 // Last ScopedBlockingCall instantiated on this thread.
-LazyInstance<ThreadLocalPointer<ScopedBlockingCall>>::Leaky
+LazyInstance<ThreadLocalPointer<internal::UncheckedScopedBlockingCall>>::Leaky
     tls_last_scoped_blocking_call = LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
-ScopedBlockingCall::ScopedBlockingCall(BlockingType blocking_type)
+namespace internal {
+
+UncheckedScopedBlockingCall::UncheckedScopedBlockingCall(
+    BlockingType blocking_type)
     : blocking_observer_(tls_blocking_observer.Get().Get()),
       previous_scoped_blocking_call_(tls_last_scoped_blocking_call.Get().Get()),
       is_will_block_(blocking_type == BlockingType::WILL_BLOCK ||
@@ -38,15 +42,28 @@ ScopedBlockingCall::ScopedBlockingCall(BlockingType blocking_type)
   }
 }
 
-ScopedBlockingCall::~ScopedBlockingCall() {
+UncheckedScopedBlockingCall::~UncheckedScopedBlockingCall() {
   DCHECK_EQ(this, tls_last_scoped_blocking_call.Get().Get());
   tls_last_scoped_blocking_call.Get().Set(previous_scoped_blocking_call_);
   if (blocking_observer_ && !previous_scoped_blocking_call_)
     blocking_observer_->BlockingEnded();
 }
 
+}  // namespace internal
+
+ScopedBlockingCall::ScopedBlockingCall(BlockingType blocking_type)
+    : UncheckedScopedBlockingCall(blocking_type) {
+  base::AssertBlockingAllowed();
+}
+
 namespace internal {
 
+ScopedBlockingCallWithBaseSyncPrimitives::
+    ScopedBlockingCallWithBaseSyncPrimitives(BlockingType blocking_type)
+    : UncheckedScopedBlockingCall(blocking_type) {
+  internal::AssertBaseSyncPrimitivesAllowed();
+}
+
 void SetBlockingObserverForCurrentThread(BlockingObserver* blocking_observer) {
   DCHECK(!tls_blocking_observer.Get().Get());
   tls_blocking_observer.Get().Set(blocking_observer);

base/threading/scoped_blocking_call.h

@@ -21,13 +21,37 @@ enum class BlockingType {
 };
 
 namespace internal {
+
 class BlockingObserver;
-}
+
+// Common implementation class for both ScopedBlockingCall and
+// ScopedBlockingCallWithBaseSyncPrimitives without assertions.
+class BASE_EXPORT UncheckedScopedBlockingCall {
+ public:
+  UncheckedScopedBlockingCall(BlockingType blocking_type);
+  ~UncheckedScopedBlockingCall();
+
+ private:
+  internal::BlockingObserver* const blocking_observer_;
+
+  // Previous ScopedBlockingCall instantiated on this thread.
+  UncheckedScopedBlockingCall* const previous_scoped_blocking_call_;
+
+  // Whether the BlockingType of the current thread was WILL_BLOCK after this
+  // ScopedBlockingCall was instantiated.
+  const bool is_will_block_;
+
+  DISALLOW_COPY_AND_ASSIGN(UncheckedScopedBlockingCall);
+};
+
+}  // namespace internal
 
 // This class must be instantiated in every scope where a blocking call is made.
-// CPU usage should be minimal within that scope. //base APIs that block
-// instantiate their own ScopedBlockingCall; it is not necessary to instantiate
-// another ScopedBlockingCall in the scope where these APIs are used. Nested
+// When a ScopedBlockingCall is instantiated, it asserts that blocking calls are
+// allowed in its scope with a call to base::AssertBlockingAllowed(). CPU usage
+// should be minimal within that scope. //base APIs that block instantiate their
+// own ScopedBlockingCall; it is not necessary to instantiate another
+// ScopedBlockingCall in the scope where these APIs are used. Nested
 // ScopedBlockingCalls are supported (mostly a no-op except for WILL_BLOCK
 // nested within MAY_BLOCK which will result in immediate WILL_BLOCK semantics).
 //
@@ -74,26 +98,27 @@ class BlockingObserver;
 // When a ScopedBlockingCall is instantiated from a TaskScheduler parallel or
 // sequenced task, the thread pool size is incremented to compensate for the
 // blocked thread (more or less aggressively depending on BlockingType).
-class BASE_EXPORT ScopedBlockingCall {
+class BASE_EXPORT ScopedBlockingCall
+    : public internal::UncheckedScopedBlockingCall {
  public:
   ScopedBlockingCall(BlockingType blocking_type);
-  ~ScopedBlockingCall();
-
- private:
-  internal::BlockingObserver* const blocking_observer_;
-
-  // Previous ScopedBlockingCall instantiated on this thread.
-  ScopedBlockingCall* const previous_scoped_blocking_call_;
-
-  // Whether the BlockingType of the current thread was WILL_BLOCK after this
-  // ScopedBlockingCall was instantiated.
-  const bool is_will_block_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedBlockingCall);
+  ~ScopedBlockingCall() = default;
 };
 
 namespace internal {
 
+// This class must be instantiated in every scope where a sync primitive is
+// used. When a ScopedBlockingCallWithBaseSyncPrimitives is instantiated, it
+// asserts that sync primitives are allowed in its scope with a call to
+// internal::AssertBaseSyncPrimitivesAllowed(). The same guidelines as for
+// ScopedBlockingCall should be followed.
+class BASE_EXPORT ScopedBlockingCallWithBaseSyncPrimitives
+    : public UncheckedScopedBlockingCall {
+ public:
+  ScopedBlockingCallWithBaseSyncPrimitives(BlockingType blocking_type);
+  ~ScopedBlockingCallWithBaseSyncPrimitives() = default;
+};
+
 // Interface for an observer to be informed when a thread enters or exits
 // the scope of ScopedBlockingCall objects.
 class BASE_EXPORT BlockingObserver {

chrome/browser/chromeos/policy/user_network_configuration_updater_factory_browsertest.cc

@@ -7,6 +7,7 @@
 #include "base/command_line.h"
 #include "base/message_loop/message_loop_current.h"
 #include "base/run_loop.h"
+#include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -179,6 +180,7 @@ void IsCertInNSSDatabaseOnIOThreadWithCertDb(
     base::OnceClosure done_closure,
     net::NSSCertDatabase* cert_db) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  base::ScopedAllowBlockingForTesting scoped_allow_blocking_for_testing;
   net::ScopedCERTCertificateList certs = cert_db->ListCertsSync();
   for (const net::ScopedCERTCertificate& cert : certs) {
     if (HasSubjectCommonName(cert.get(), subject_common_name)) {