Do not flush TaskScheduler in ~TestBrowserThreadBundle if it's not managed by it.

This fixes hangs in https://codereview.chromium.org/2890853002/.

New regression test hangs without matching test_browser_thread.cc 
changes.

We support adding a TestBrowserThreadBundle on top of an explicit
ScopedTaskEnvironment (i.e. to add named threads in unit tests that
already otherwise inherit a ScopedTaskEnvironment or need to provide a
custom one). However if the ScopedTaskEnvironment uses a QUEUED mode,
trying to flush it in ~TestBrowserThreadBundle() results in a hang.
Flushing TaskScheduler when it's not managed by that
TestBrowserThreadBundle was wrong anyways.

Bug: 708584, 689520
Change-Id: I3087537ad4f1065fc974170b00c25a676ff5e4c9
Reviewed-on: https://chromium-review.googlesource.com/575593Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487481}

content/public/test/test_browser_thread_bundle.cc

@@ -63,20 +63,28 @@ TestBrowserThreadBundle::~TestBrowserThreadBundle() {
   ui_thread_->Stop();
   base::RunLoop().RunUntilIdle();
 
-  // This is required to ensure we run all remaining tasks in an atomic step
-  // (instead of ~ScopedAsyncTaskScheduler() followed by another
-  // RunLoop().RunUntilIdle()). Otherwise If a pending task in
-  // |scoped_async_task_scheduler_| posts to |message_loop_|, that task can then
-  // post back to |scoped_async_task_scheduler_| after the former was destroyed.
-  // This is a bit different than production where the main thread is not
-  // flushed after it's done running but this approach is preferred in unit
-  // tests as running more tasks can merely uncover more issues (e.g. if a bad
-  // tasks is posted but never blocked upon it could make a test flaky whereas
-  // by flushing we guarantee it will blow up).
-  RunAllBlockingPoolTasksUntilIdle();
-
-  scoped_async_task_scheduler_.reset();
-  CHECK(base::MessageLoop::current()->IsIdleForTesting());
+  // Skip the following step when TaskScheduler isn't managed by this
+  // TestBrowserThreadBundle, otherwise it can hang (e.g.
+  // RunAllBlockingPoolTasksUntilIdle() hangs when the TaskScheduler is managed
+  // by a ScopedTaskEnvironment with ExecutionMode::QUEUED). This is fine as (1)
+  // it's rare and (2) it mimics production where BrowserThreads are shutdown
+  // before TaskScheduler.
+  if (scoped_async_task_scheduler_) {
+    // This is required to ensure we run all remaining tasks in an atomic step
+    // (instead of ~ScopedAsyncTaskScheduler() followed by another
+    // RunLoop().RunUntilIdle()). Otherwise If a pending task in
+    // |scoped_async_task_scheduler_| posts to |message_loop_|, that task can
+    // then post back to |scoped_async_task_scheduler_| after the former was
+    // destroyed. This is a bit different than production where the main thread
+    // is not flushed after it's done running but this approach is preferred in
+    // unit tests as running more tasks can merely uncover more issues (e.g. if
+    // a bad tasks is posted but never blocked upon it could make a test flaky
+    // whereas by flushing we guarantee it will blow up).
+    RunAllBlockingPoolTasksUntilIdle();
+
+    scoped_async_task_scheduler_.reset();
+    CHECK(base::MessageLoop::current()->IsIdleForTesting());
+  }
 
   // |message_loop_| needs to explicitly go away before fake threads in order
   // for DestructionObservers hooked to |message_loop_| to be able to invoke

content/public/test/test_browser_thread_bundle_unittest.cc

@@ -4,16 +4,42 @@
 
 #include "content/public/test/test_browser_thread_bundle.h"
 
+#include "base/bind_helpers.h"
 #include "base/message_loop/message_loop.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/test/scoped_task_environment.h"
+#include "content/public/browser/browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::test::ScopedTaskEnvironment;
+
 namespace content {
 
-TEST(TestBrowserThreadBundle, ScopedTaskEnvironmentAndTestBrowserThreadBundle) {
-  base::test::ScopedTaskEnvironment scoped_task_environment(
-      base::test::ScopedTaskEnvironment::MainThreadType::UI);
+TEST(TestBrowserThreadBundleTest,
+     ScopedTaskEnvironmentAndTestBrowserThreadBundle) {
+  ScopedTaskEnvironment scoped_task_environment(
+      ScopedTaskEnvironment::MainThreadType::UI);
   TestBrowserThreadBundle test_browser_thread_bundle;
+  base::PostTaskAndReply(
+      FROM_HERE, base::BindOnce(&base::DoNothing),
+      base::BindOnce([]() { DCHECK_CURRENTLY_ON(BrowserThread::UI); }));
+  scoped_task_environment.RunUntilIdle();
+}
+
+// Regression test to verify that ~TestBrowserThreadBundle() doesn't hang when
+// the TaskScheduler is owned by a QUEUED ScopedTaskEnvironment with pending
+// tasks.
+TEST(TestBrowserThreadBundleTest,
+     QueuedScopedTaskEnvironmentAndTestBrowserThreadBundle) {
+  ScopedTaskEnvironment queued_scoped_task_environment(
+      ScopedTaskEnvironment::MainThreadType::UI,
+      ScopedTaskEnvironment::ExecutionMode::QUEUED);
+  base::PostTask(FROM_HERE, base::BindOnce(&base::DoNothing));
+
+  {
+    TestBrowserThreadBundle test_browser_thread_bundle;
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  }  // Would hang here prior to fix.
 }
 
 TEST(TestBrowserThreadBundleTest, MessageLoopTypeMismatch) {