Improve usage documentation of scoped task environments.

And improve information provided by out-of-order destruction of task
scheduler and its provided constructs in unit tests.

(comes out of discussion on https://codereview.chromium.org/2852333004/)

BUG=708584, 689520
TBR=avi@chromium.org (documentation update in test_browser_thread_bundle.h)

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

base/task_scheduler/scheduler_single_thread_task_runner_manager.cc

@@ -11,6 +11,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/debug/stack_trace.h"
 #include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
@@ -284,6 +285,11 @@ class SchedulerSingleThreadTaskRunnerManager::SchedulerSingleThreadTaskRunner
 
  private:
   ~SchedulerSingleThreadTaskRunner() override {
+    // Note: This will crash if SchedulerSingleThreadTaskRunnerManager is
+    // incorrectly destroyed first in tests (in production the TaskScheduler and
+    // all of its state are intentionally leaked after
+    // TaskScheduler::Shutdown(). See ~SchedulerSingleThreadTaskRunnerManager()
+    // for more details.
     outer_->UnregisterSchedulerWorker(worker_);
   }
 
@@ -325,10 +331,29 @@ SchedulerSingleThreadTaskRunnerManager::
 #if DCHECK_IS_ON()
   size_t workers_unregistered_during_join =
       subtle::NoBarrier_Load(&workers_unregistered_during_join_);
-  DCHECK_EQ(workers_unregistered_during_join, workers_.size())
-      << "There cannot be outstanding SingleThreadTaskRunners upon destruction "
-         "of SchedulerSingleThreadTaskRunnerManager or the Task Scheduler";
-#endif
+  // Log an ERROR instead of DCHECK'ing as it's often useful to have both the
+  // stack trace of this call and the crash stack trace of the upcoming
+  // out-of-order ~SchedulerSingleThreadTaskRunner() call to know what to flip.
+  DLOG_IF(ERROR, workers_unregistered_during_join != workers_.size())
+      << "Expect incoming crash in ~SchedulerSingleThreadTaskRunner()!!! There "
+         "cannot be outstanding SingleThreadTaskRunners upon destruction "
+         "of SchedulerSingleThreadTaskRunnerManager in tests "
+      << workers_.size() - workers_unregistered_during_join << " outstanding). "
+      << "Hint 1: If you're hitting this it's most likely because your test "
+         "fixture is destroying its TaskScheduler too early (e.g. via "
+         "base::test::~ScopedTaskEnvironment() or "
+         "content::~TestBrowserThreadBundle()). Refer to the following stack "
+         "trace to know what caused this destruction as well as to the "
+         "upcoming crash in ~SchedulerSingleThreadTaskRunner() to know what "
+         "should have happened before. "
+         "Hint 2: base::test::ScopedTaskEnvironment et al. should typically "
+         "be the first member in a test fixture to ensure it's initialized "
+         "first and destroyed last.\n"
+#if !defined(OS_NACL)  // We don't build base/debug/stack_trace.cc for NaCl.
+      << base::debug::StackTrace().ToString()
+#endif  // !defined(OS_NACL)
+      ;
+#endif  // DCHECK_IS_ON()
 }
 
 void SchedulerSingleThreadTaskRunnerManager::Start() {

base/test/scoped_task_environment.h

@@ -28,6 +28,23 @@ namespace test {
 // Tasks posted through base/task_scheduler/post_task.h run on dedicated threads
 // as they are posted.
 //
+// Usage:
+//
+//   class MyTestFixture : public testing::Test {
+//    public:
+//     (...)
+//
+//    protected:
+//     // Must be the first member (or at least before any member that cares
+//     // about tasks) to be initialized first and destroyed last. protected
+//     // instead of private visibility will allow controlling the task
+//     // environment (e.g. clock) once such features are added (see design doc
+//     // below for details), until then it at least doesn't hurt :).
+//     base::test::ScopedTaskEnvironment scoped_task_environment_;
+//
+//     // Other members go here (or further below in private section.)
+//   };
+//
 // Design and future improvements documented in
 // https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3Pkk/edit
 class ScopedTaskEnvironment {

content/public/test/test_browser_thread_bundle.h

@@ -53,6 +53,24 @@
 //
 // DONT_CREATE_THREADS should only be used when the options specify at least
 // one real thread other than the main thread.
+//
+// Basic usage:
+//
+//   class MyTestFixture : public testing::Test {
+//    public:
+//     (...)
+//
+//    protected:
+//     // Must be the first member (or at least before any member that cares
+//     // about tasks) to be initialized first and destroyed last. protected
+//     // instead of private visibility will allow controlling the task
+//     // environment (e.g. clock) once such features are added (see
+//     // base::test::ScopedTaskEnvironment for details), until then it at least
+//     // doesn't hurt :).
+//     content::TestBrowserThreadBundle test_browser_thread_bundle_;
+//
+//     // Other members go here (or further below in private section.)
+//   };
 
 #ifndef CONTENT_PUBLIC_TEST_TEST_BROWSER_THREAD_BUNDLE_H_
 #define CONTENT_PUBLIC_TEST_TEST_BROWSER_THREAD_BUNDLE_H_