Exempt the Service Thread from BLOCK_SHUTDOWN DCHECKs

In production, FileDescriptorWatchers can be leaky and post
BLOCK_SHUTDOWN tasks in response to their notification.

Stopping the service thread requires some non-trivial task lifetime
fixes to do it correctly, so this was deemed the next best quick fix
for redirection.

BUG=717380

Review-Url: https://codereview.chromium.org/2857103005
Cr-Commit-Position: refs/heads/master@{#469461}

base/task_scheduler/task_scheduler_impl.cc

@@ -104,7 +104,11 @@ void TaskSchedulerImpl::Start(const TaskScheduler::InitParams& init_params) {
   // message_loop().
   task_tracker_.set_watch_file_descriptor_message_loop(
       static_cast<MessageLoopForIO*>(service_thread_.message_loop()));
-#endif
+
+#if DCHECK_IS_ON()
+  task_tracker_.set_service_thread_handle(service_thread_.GetThreadHandle());
+#endif  // DCHECK_IS_ON()
+#endif  // defined(OS_POSIX) && !defined(OS_NACL_SFI)
 
   // Needs to happen after starting the service thread to get its task_runner().
   delayed_task_manager_.Start(service_thread_.task_runner());

base/task_scheduler/task_scheduler_impl_unittest.cc

@@ -26,8 +26,18 @@
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
+#include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_POSIX)
+#include <unistd.h>
+
+#include "base/debug/leak_annotations.h"
+#include "base/files/file_descriptor_watcher_posix.h"
+#include "base/files/file_util.h"
+#include "base/posix/eintr_wrapper.h"
+#endif  // defined(OS_POSIX)
+
 #if defined(OS_WIN)
 #include <objbase.h>
 #endif  // defined(OS_WIN)
@@ -447,5 +457,74 @@ TEST_F(TaskSchedulerImplTest, COMSTATaskRunnersRunWithCOMSTA) {
 }
 #endif  // defined(OS_WIN)
 
+TEST_F(TaskSchedulerImplTest, DelayedTasksNotRunAfterShutdown) {
+  StartTaskScheduler();
+  // As with delayed tasks in general, this is racy. If the task does happen to
+  // run after Shutdown within the timeout, it will fail this test.
+  //
+  // The timeout should be set sufficiently long enough to ensure that the
+  // delayed task did not run. 2x is generally good enough.
+  //
+  // A non-racy way to do this would be to post two sequenced tasks:
+  // 1) Regular Post Task: A WaitableEvent.Wait
+  // 2) Delayed Task: ADD_FAILURE()
+  // and signalling the WaitableEvent after Shutdown() on a different thread
+  // since Shutdown() will block. However, the cost of managing this extra
+  // thread was deemed to be too great for the unlikely race.
+  scheduler_.PostDelayedTaskWithTraits(FROM_HERE, TaskTraits(),
+                                       BindOnce([]() { ADD_FAILURE(); }),
+                                       TestTimeouts::tiny_timeout());
+  scheduler_.Shutdown();
+  PlatformThread::Sleep(TestTimeouts::tiny_timeout() * 2);
+}
+
+#if defined(OS_POSIX)
+
+TEST_F(TaskSchedulerImplTest, FileDescriptorWatcherNoOpsAfterShutdown) {
+  StartTaskScheduler();
+
+  int pipes[2];
+  ASSERT_EQ(0, pipe(pipes));
+
+  scoped_refptr<TaskRunner> blocking_task_runner =
+      scheduler_.CreateSequencedTaskRunnerWithTraits(
+          TaskTraits().WithShutdownBehavior(
+              TaskShutdownBehavior::BLOCK_SHUTDOWN));
+  blocking_task_runner->PostTask(
+      FROM_HERE,
+      BindOnce(
+          [](int read_fd) {
+            std::unique_ptr<FileDescriptorWatcher::Controller> controller =
+                FileDescriptorWatcher::WatchReadable(
+                    read_fd, BindRepeating([]() { NOTREACHED(); }));
+
+            // This test is for components that intentionally leak their
+            // watchers at shutdown. We can't clean |controller| up because its
+            // destructor will assert that it's being called from the correct
+            // sequence. After the task scheduler is shutdown, it is not
+            // possible to run tasks on this sequence.
+            //
+            // Note: Do not inline the controller.release() call into the
+            //       ANNOTATE_LEAKING_OBJECT_PTR as the annotation is removed
+            //       by the preprocessor in non-LEAK_SANITIZER builds,
+            //       effectively breaking this test.
+            ANNOTATE_LEAKING_OBJECT_PTR(controller.get());
+            controller.release();
+          },
+          pipes[0]));
+
+  scheduler_.Shutdown();
+
+  constexpr char kByte = '!';
+  ASSERT_TRUE(WriteFileDescriptor(pipes[1], &kByte, sizeof(kByte)));
+
+  // Give a chance for the file watcher to fire before closing the handles.
+  PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+
+  EXPECT_EQ(0, IGNORE_EINTR(close(pipes[0])));
+  EXPECT_EQ(0, IGNORE_EINTR(close(pipes[1])));
+}
+#endif  // defined(OS_POSIX)
+
 }  // namespace internal
 }  // namespace base

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"
@@ -383,6 +382,12 @@ void TaskTracker::PerformShutdown() {
   }
 }
 
+#if DCHECK_IS_ON()
+bool TaskTracker::IsPostingBlockShutdownTaskAfterShutdownAllowed() {
+  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
@@ -395,7 +400,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.
+#if DCHECK_IS_ON()
+        DCHECK(IsPostingBlockShutdownTaskAfterShutdownAllowed());
+#endif
+        state_->DecrementNumTasksBlockingShutdown();
+        return false;
+      }
 
       ++num_block_shutdown_tasks_posted_during_shutdown_;
 

base/task_scheduler/task_tracker.h

@@ -10,6 +10,7 @@
 #include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/callback_forward.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_base.h"
 #include "base/synchronization/waitable_event.h"
@@ -80,6 +81,13 @@ class BASE_EXPORT TaskTracker {
   // but is free to perform extra work before and after doing so.
   virtual void PerformRunTask(std::unique_ptr<Task> task);
 
+#if DCHECK_IS_ON()
+  // Returns true if this context should be exempt from blocking shutdown
+  // DCHECKs.
+  // TODO(robliao): Remove when http://crbug.com/698140 is fixed.
+  virtual bool IsPostingBlockShutdownTaskAfterShutdownAllowed();
+#endif
+
  private:
   class State;
 

base/task_scheduler/task_tracker_posix.cc

@@ -7,7 +7,6 @@
 #include <utility>
 
 #include "base/files/file_descriptor_watcher_posix.h"
-#include "base/logging.h"
 
 namespace base {
 namespace internal {
@@ -22,5 +21,11 @@ void TaskTrackerPosix::PerformRunTask(std::unique_ptr<Task> task) {
   TaskTracker::PerformRunTask(std::move(task));
 }
 
+#if DCHECK_IS_ON()
+bool TaskTrackerPosix::IsPostingBlockShutdownTaskAfterShutdownAllowed() {
+  return service_thread_handle_.is_equal(PlatformThread::CurrentHandle());
+}
+#endif
+
 }  // namespace internal
 }  // namespace base

base/task_scheduler/task_tracker_posix.h

@@ -8,8 +8,10 @@
 #include <memory>
 
 #include "base/base_export.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/task_scheduler/task_tracker.h"
+#include "base/threading/platform_thread.h"
 
 namespace base {
 
@@ -37,12 +39,31 @@ class BASE_EXPORT TaskTrackerPosix : public TaskTracker {
     watch_file_descriptor_message_loop_ = watch_file_descriptor_message_loop;
   }
 
+#if DCHECK_IS_ON()
+  // TODO(robliao): http://crbug.com/698140. This addresses service thread tasks
+  // that could run after the task scheduler has shut down. Anything from the
+  // service thread is exempted from the task scheduler shutdown DCHECKs.
+  void set_service_thread_handle(
+      const PlatformThreadHandle& service_thread_handle) {
+    DCHECK(!service_thread_handle.is_null());
+    service_thread_handle_ = service_thread_handle;
+  }
+#endif
+
  private:
   // TaskTracker:
   void PerformRunTask(std::unique_ptr<Task> task) override;
 
+#if DCHECK_IS_ON()
+  bool IsPostingBlockShutdownTaskAfterShutdownAllowed() override;
+#endif
+
   MessageLoopForIO* watch_file_descriptor_message_loop_ = nullptr;
 
+#if DCHECK_IS_ON()
+  PlatformThreadHandle service_thread_handle_;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(TaskTrackerPosix);
 };
 

base/threading/thread.cc

@@ -225,6 +225,11 @@ PlatformThreadId Thread::GetThreadId() const {
   return id_;
 }
 
+PlatformThreadHandle Thread::GetThreadHandle() const {
+  AutoLock lock(thread_lock_);
+  return thread_;
+}
+
 bool Thread::IsRunning() const {
   // TODO(gab): Fix improper usage of this API (http://crbug.com/629139) and
   // enable this check.

base/threading/thread.h

@@ -245,6 +245,15 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   // This method is thread-safe.
   PlatformThreadId GetThreadId() const;
 
+  // Returns the current thread handle. If called before Start*() returns or
+  // after Stop() returns, an empty thread handle will be returned.
+  //
+  // This method is thread-safe.
+  //
+  // TODO(robliao): Remove this when it no longer needs to be temporarily
+  // exposed for http://crbug.com/717380.
+  PlatformThreadHandle GetThreadHandle() const;
+
   // Returns true if the thread has been started, and not yet stopped.
   bool IsRunning() const;