Exempt the Service Thread from BLOCK_SHUTDOWN DCHECKs

base/task_scheduler/task_tracker.cc

Index: base/task_scheduler/task_tracker.cc
diff --git a/base/task_scheduler/task_tracker.cc b/base/task_scheduler/task_tracker.cc
index da21d5ea8ef510c33636cc993b34e3501aa09101..c7ff4d499ac1c62a2578999a351b101dd3637e2d 100644
--- a/base/task_scheduler/task_tracker.cc
+++ b/base/task_scheduler/task_tracker.cc
@@ -10,7 +10,6 @@
 #include "base/callback.h"
 #include "base/debug/task_annotator.h"
 #include "base/json/json_writer.h"
-#include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequence_token.h"
@@ -379,6 +378,12 @@ void TaskTracker::PerformShutdown() {
   }
 }
 
+#if DCHECK_IS_ON()
+bool TaskTracker::IsExemptFromBlockingShutdownChecks() {
+  return false;
+}
+#endif
+
 bool TaskTracker::BeforePostTask(TaskShutdownBehavior shutdown_behavior) {
   if (shutdown_behavior == TaskShutdownBehavior::BLOCK_SHUTDOWN) {
     // BLOCK_SHUTDOWN tasks block shutdown between the moment they are posted
@@ -391,7 +396,17 @@ bool TaskTracker::BeforePostTask(TaskShutdownBehavior shutdown_behavior) {
       // A BLOCK_SHUTDOWN task posted after shutdown has completed is an
       // ordering bug. This aims to catch those early.
       DCHECK(shutdown_event_);
-      DCHECK(!shutdown_event_->IsSignaled());
+      if (shutdown_event_->IsSignaled()) {
+        // TODO(robliao): http://crbug.com/698140. Since the service thread
+        // doesn't stop processing its own tasks at shutdown, we may still
+        // attempt to post a BLOCK_SHUTDOWN task in response to a
+        // FileDescriptorWatcher.

[gab, 2017/05/04 17:36:11]

Indicate that while these tasks shouldn't be BLOCK_SHUTDOWN since they have no
guarantees of firing in the first place, they often are because they're posted
on a sequence that is. This will be solved by sequence funneling.

I think the TODO (and documentation in this CL) should be around that more (i.e.
I don't think we care to fix it as those tasks aren't truly BLOCK_SHUTDOWN,
instead we want to properly post them as SKIP_ON_SHUTDOWN when that becomes
possible)

[robliao, 2017/05/04 17:43:24]

We should keep the extra details in the bug, especially implementation details
that may change and outdate any further comments here. This means we should keep
the comments here factual on the state of the world when we introduced this
code.

Sequence funneling will not inherently solve the issue as we still need to fix
all of the callers that register these FileDescriptorWatchers.

+#if DCHECK_IS_ON()
+        DCHECK(IsExemptFromBlockingShutdownChecks());

[gab, 2017/05/04 18:11:58]

The log "DCHECK(!shutdown_event_->IsSignaled());" was previously more obvious
and since people hit this occasionally it should remain easy to see what's going
on whereas "DCHECK(IsExemptFromBlockingShutdownChecks());" isn't as obvious to
the untrained IMO.

How about

DCHECK(IsPostingBlockShutdownTaskAfterShutdownAllowed());

or something?

[robliao, 2017/05/04 19:01:49]

sgtm. Done.

+#endif
+        state_->DecrementNumTasksBlockingShutdown();
+        return false;
+      }
 
       ++num_block_shutdown_tasks_posted_during_shutdown_;