Revert of Tests: Simplify SequencedWorkerPoolOwner, call Shutdown on …

…destructor. (patchset chromium#4 id:60001 of https://codereview.chromium.org/1417353006/ )

Reason for revert:
Suspected for persistent failures on ios_net_unittests

- RoundTripTestCookieStore/CookieStoreTest/0.TestNonDottedAndTLD
- CookieStoreIOS/CookieStoreTest/0.TestNonDottedAndTLD

Failing since https://build.chromium.org/p/chromium.mac/builders/iOS_Simulator_%28dbg%29/builds/32876

Errors like

../../net/cookies/cookie_store_unittest.h:536: Failure
Value of: this->SetCookie(cs.get(), url, "a=1; domain=com")
  Actual: false
Expected: true

(It's a tricky one! Sorry if this is not it)

Original issue's description:
> Tests: Simplify SequencedWorkerPoolOwner, call Shutdown on destructor.
>
> Also ports remaining tests using raw SWPs to use SWPOwner.
>
> BUG=450228
>
> Committed: https://crrev.com/fc939726c283e34112d9bc845a39460410fe9cd9
> Cr-Commit-Position: refs/heads/master@{#362805}

TBR=phajdan.jr@chromium.org,jam@chromium.org,brettw@chromium.org,tommycli@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=450228

Review URL: https://codereview.chromium.org/1496493004

Cr-Commit-Position: refs/heads/master@{#362890}

base/sequence_checker_unittest.cc

@@ -66,6 +66,7 @@ class SequenceCheckerTest : public testing::Test {
 
   void TearDown() override {
     other_thread_.Stop();
+    pool()->Shutdown();
   }
 
  protected:
@@ -308,6 +309,7 @@ void SequenceCheckerTest::TwoDifferentWorkerPoolsDeathTest() {
       base::Bind(&SequenceCheckedObject::DoStuff,
                  base::Unretained(sequence_checked_object.get())));
   second_pool_owner.pool()->FlushForTesting();
+  second_pool_owner.pool()->Shutdown();
 }
 
 #if ENABLE_SEQUENCE_CHECKER

base/test/launcher/test_launcher.cc

@@ -468,7 +468,10 @@ TestLauncher::TestLauncher(TestLauncherDelegate* launcher_delegate,
       parallel_jobs_(parallel_jobs) {
 }
 
-TestLauncher::~TestLauncher() {}
+TestLauncher::~TestLauncher() {
+  if (worker_pool_owner_)
+    worker_pool_owner_->pool()->Shutdown();
+}
 
 bool TestLauncher::Run() {
   if (!Init())

base/test/sequenced_worker_pool_owner.cc

@@ -18,11 +18,8 @@ SequencedWorkerPoolOwner::SequencedWorkerPoolOwner(
       has_work_call_count_(0) {}
 
 SequencedWorkerPoolOwner::~SequencedWorkerPoolOwner() {
-  pool_->Shutdown();
   pool_ = NULL;
-
-  // Spin the current message loop until SWP destruction verified in OnDestruct.
-  exit_loop_.Run();
+  MessageLoop::current()->Run();
 }
 
 const scoped_refptr<SequencedWorkerPool>& SequencedWorkerPoolOwner::pool() {
@@ -54,7 +51,8 @@ void SequencedWorkerPoolOwner::WillWaitForShutdown() {
 }
 
 void SequencedWorkerPoolOwner::OnDestruct() {
-  constructor_message_loop_->PostTask(FROM_HERE, exit_loop_.QuitClosure());
+  constructor_message_loop_->task_runner()->PostTask(
+      FROM_HERE, constructor_message_loop_->QuitWhenIdleClosure());
 }
 
 }  // namespace base

base/test/sequenced_worker_pool_owner.h

@@ -12,7 +12,6 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
-#include "base/run_loop.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/sequenced_worker_pool.h"
 
@@ -26,9 +25,6 @@ class MessageLoop;
 // strange races with other tests that touch global stuff (like histograms and
 // logging).  However, this requires that nothing else on this thread holds a
 // ref to the pool when the SequencedWorkerPoolOwner is destroyed.
-//
-// This class calls Shutdown on the owned SequencedWorkerPool in the destructor.
-// Tests may themselves call Shutdown earlier to test shutdown behavior.
 class SequencedWorkerPoolOwner : public SequencedWorkerPool::TestingObserver {
  public:
   SequencedWorkerPoolOwner(size_t max_threads,
@@ -50,11 +46,7 @@ class SequencedWorkerPoolOwner : public SequencedWorkerPool::TestingObserver {
   void WillWaitForShutdown() override;
   void OnDestruct() override;
 
-  // Used to run the current thread's message loop until the
-  // SequencedWorkerPool's destruction has been verified.
-  base::RunLoop exit_loop_;
   MessageLoop* const constructor_message_loop_;
-
   scoped_refptr<SequencedWorkerPool> pool_;
   Closure will_wait_for_shutdown_callback_;
 

base/threading/sequenced_task_runner_handle_unittest.cc

@@ -61,6 +61,7 @@ TEST_F(SequencedTaskRunnerHandleTest, FromSequencedWorkerPool) {
       base::Bind(&SequencedTaskRunnerHandleTest::GetTaskRunner,
                  base::Bind(&WaitableEvent::Signal, base::Unretained(&event))));
   event.Wait();
+  owner.pool()->Shutdown();
 }
 
 class ThreadRunner : public DelegateSimpleThread::Delegate {

base/threading/sequenced_worker_pool_unittest.cc

@@ -234,6 +234,8 @@ class SequencedWorkerPoolTest : public testing::Test {
     ResetPool();
   }
 
+  void TearDown() override { pool()->Shutdown(); }
+
   const scoped_refptr<SequencedWorkerPool>& pool() {
     return pool_owner_->pool();
   }
@@ -349,6 +351,7 @@ TEST_F(SequencedWorkerPoolTest, DelayedTaskDuringShutdown) {
   ASSERT_EQ(1u, completion_sequence.size());
   ASSERT_EQ(1, completion_sequence[0]);
 
+  pool()->Shutdown();
   // Shutdown is asynchronous, so use ResetPool() to block until the pool is
   // fully destroyed (and thus shut down).
   ResetPool();
@@ -427,6 +430,9 @@ TEST_F(SequencedWorkerPoolTest, LotsOfTasksTwoPools) {
   std::vector<int> result =
       tracker()->WaitUntilTasksComplete(2*kNumTasks);
   EXPECT_EQ(2 * kNumTasks, result.size());
+
+  pool2.pool()->Shutdown();
+  pool1.pool()->Shutdown();
 }
 
 // Test that tasks with the same sequence token are executed in order but don't
@@ -793,30 +799,33 @@ TEST_F(SequencedWorkerPoolTest, IsRunningOnCurrentThread) {
   SequencedWorkerPool::SequenceToken token2 = pool()->GetSequenceToken();
   SequencedWorkerPool::SequenceToken unsequenced_token;
 
-  SequencedWorkerPoolOwner unused_pool_owner(2, "unused_pool");
+  scoped_refptr<SequencedWorkerPool> unused_pool =
+      new SequencedWorkerPool(2, "unused_pool");
 
   EXPECT_FALSE(pool()->RunsTasksOnCurrentThread());
   EXPECT_FALSE(pool()->IsRunningSequenceOnCurrentThread(token1));
   EXPECT_FALSE(pool()->IsRunningSequenceOnCurrentThread(token2));
   EXPECT_FALSE(pool()->IsRunningSequenceOnCurrentThread(unsequenced_token));
-  EXPECT_FALSE(unused_pool_owner.pool()->RunsTasksOnCurrentThread());
-  EXPECT_FALSE(
-      unused_pool_owner.pool()->IsRunningSequenceOnCurrentThread(token1));
+  EXPECT_FALSE(unused_pool->RunsTasksOnCurrentThread());
+  EXPECT_FALSE(unused_pool->IsRunningSequenceOnCurrentThread(token1));
+  EXPECT_FALSE(unused_pool->IsRunningSequenceOnCurrentThread(token2));
   EXPECT_FALSE(
-      unused_pool_owner.pool()->IsRunningSequenceOnCurrentThread(token2));
-  EXPECT_FALSE(unused_pool_owner.pool()->IsRunningSequenceOnCurrentThread(
-      unsequenced_token));
+      unused_pool->IsRunningSequenceOnCurrentThread(unsequenced_token));
 
   pool()->PostSequencedWorkerTask(
-      token1, FROM_HERE, base::Bind(&IsRunningOnCurrentThreadTask, token1,
-                                    token2, pool(), unused_pool_owner.pool()));
+      token1, FROM_HERE,
+      base::Bind(&IsRunningOnCurrentThreadTask,
+                 token1, token2, pool(), unused_pool));
   pool()->PostSequencedWorkerTask(
       token2, FROM_HERE,
-      base::Bind(&IsRunningOnCurrentThreadTask, token2, unsequenced_token,
-                 pool(), unused_pool_owner.pool()));
+      base::Bind(&IsRunningOnCurrentThreadTask,
+                 token2, unsequenced_token, pool(), unused_pool));
   pool()->PostWorkerTask(
-      FROM_HERE, base::Bind(&IsRunningOnCurrentThreadTask, unsequenced_token,
-                            token1, pool(), unused_pool_owner.pool()));
+      FROM_HERE,
+      base::Bind(&IsRunningOnCurrentThreadTask,
+                 unsequenced_token, token1, pool(), unused_pool));
+  pool()->Shutdown();
+  unused_pool->Shutdown();
 }
 
 // Checks that tasks are destroyed in the right context during shutdown. If a
@@ -938,14 +947,16 @@ TEST_F(SequencedWorkerPoolTest, GetWorkerPoolAndSequenceTokenForCurrentThread) {
   pool()->FlushForTesting();
 }
 
-TEST_F(SequencedWorkerPoolTest, ShutsDownCleanWithContinueOnShutdown) {
+TEST(SequencedWorkerPoolRefPtrTest, ShutsDownCleanWithContinueOnShutdown) {
+  MessageLoop loop;
+  scoped_refptr<SequencedWorkerPool> pool(new SequencedWorkerPool(3, "Pool"));
   scoped_refptr<SequencedTaskRunner> task_runner =
-      pool()->GetSequencedTaskRunnerWithShutdownBehavior(
-          pool()->GetSequenceToken(),
+      pool->GetSequencedTaskRunnerWithShutdownBehavior(
+          pool->GetSequenceToken(),
           base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
 
   // Upon test exit, should shut down without hanging.
-  pool()->Shutdown();
+  pool->Shutdown();
 }
 
 class SequencedWorkerPoolTaskRunnerTestDelegate {

chrome/browser/chromeos/extensions/external_cache_unittest.cc

@@ -70,6 +70,11 @@ class ExternalCacheTest : public testing::Test,
         pool_owner_->pool()->GetNamedSequenceToken("background"));
   }
 
+  void TearDown() override {
+    pool_owner_->pool()->Shutdown();
+    base::RunLoop().RunUntilIdle();
+  }
+
   // ExternalCache::Delegate:
   void OnExtensionListsUpdated(const base::DictionaryValue* prefs) override {
     prefs_.reset(prefs->DeepCopy());

chrome/browser/extensions/updater/local_extension_cache_unittest.cc

@@ -48,6 +48,11 @@ class LocalExtensionCacheTest : public testing::Test {
         pool_owner_->pool()->GetNamedSequenceToken("background"));
   }
 
+  void TearDown() override {
+    pool_owner_->pool()->Shutdown();
+    base::RunLoop().RunUntilIdle();
+  }
+
   base::FilePath CreateCacheDir(bool initialized) {
     EXPECT_TRUE(cache_dir_.CreateUniqueTempDir());
     if (initialized)

chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc

@@ -7,7 +7,6 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
-#include "base/test/sequenced_worker_pool_owner.h"
 #include "base/thread_task_runner_handle.h"
 #include "chrome/browser/sync_file_system/drive_backend/callback_helper.h"
 #include "chrome/browser/sync_file_system/drive_backend/fake_sync_worker.h"
@@ -29,7 +28,7 @@ class SyncEngineTest : public testing::Test,
  public:
   typedef RemoteFileSyncService::OriginStatusMap RemoteOriginStatusMap;
 
-  SyncEngineTest() : worker_pool_owner_(1, "Worker") {}
+  SyncEngineTest() {}
   ~SyncEngineTest() override {}
 
   void SetUp() override {
@@ -38,25 +37,28 @@ class SyncEngineTest : public testing::Test,
     scoped_ptr<drive::DriveServiceInterface>
         fake_drive_service(new drive::FakeDriveService);
 
+    worker_pool_ = new base::SequencedWorkerPool(1, "Worker");
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner =
         base::ThreadTaskRunnerHandle::Get();
     worker_task_runner_ =
-        worker_pool_owner_.pool()->GetSequencedTaskRunnerWithShutdownBehavior(
-            worker_pool_owner_.pool()->GetSequenceToken(),
+        worker_pool_->GetSequencedTaskRunnerWithShutdownBehavior(
+            worker_pool_->GetSequenceToken(),
             base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
 
     sync_engine_.reset(new drive_backend::SyncEngine(
-        ui_task_runner.get(), worker_task_runner_.get(),
-        nullptr,  // drive_task_runner
-        worker_pool_owner_.pool().get(), profile_dir_.path(),
-        nullptr,    // task_logger
-        nullptr,    // notification_manager
-        nullptr,    // extension_service
-        nullptr,    // signin_manager
-        nullptr,    // token_service
-        nullptr,    // request_context
-        nullptr,    // drive_service_factory
-        nullptr));  // in_memory_env
+        ui_task_runner.get(),
+        worker_task_runner_.get(),
+        nullptr /* drive_task_runner */,
+        worker_pool_.get(),
+        profile_dir_.path(),
+        nullptr /* task_logger */,
+        nullptr /* notification_manager */,
+        nullptr /* extension_service */,
+        nullptr /* signin_manager */,
+        nullptr /* token_service */,
+        nullptr /* request_context */,
+        nullptr /* drive_service_factory */,
+        nullptr /* in_memory_env */));
 
     sync_engine_->InitializeForTesting(
         fake_drive_service.Pass(),
@@ -71,8 +73,12 @@ class SyncEngineTest : public testing::Test,
   void TearDown() override {
     sync_engine_.reset();
     WaitForWorkerTaskRunner();
+    worker_pool_->Shutdown();
 
     worker_task_runner_ = nullptr;
+    worker_pool_ = nullptr;
+
+    base::RunLoop().RunUntilIdle();
   }
 
   bool FindOriginStatus(const GURL& origin, std::string* status) {
@@ -125,7 +131,7 @@ class SyncEngineTest : public testing::Test,
   base::ScopedTempDir profile_dir_;
   scoped_ptr<drive_backend::SyncEngine> sync_engine_;
 
-  base::SequencedWorkerPoolOwner worker_pool_owner_;
+  scoped_refptr<base::SequencedWorkerPool> worker_pool_;
   scoped_refptr<base::SequencedTaskRunner> worker_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncEngineTest);

chrome/browser/ui/app_list/search/history_unittest.cc

@@ -99,6 +99,7 @@ class SearchHistoryTest : public testing::Test {
   }
   void TearDown() override {
     Flush();
+    worker_pool_owner_->pool()->Shutdown();
   }
 
   void CreateHistory() {

components/autofill/core/browser/test_autofill_driver.cc

@@ -14,7 +14,9 @@ TestAutofillDriver::TestAutofillDriver()
           new base::SequencedWorkerPoolOwner(4, "TestAutofillDriver")),
       url_request_context_(NULL) {}
 
-TestAutofillDriver::~TestAutofillDriver() {}
+TestAutofillDriver::~TestAutofillDriver() {
+  blocking_pool_owner_->pool()->Shutdown();
+}
 
 bool TestAutofillDriver::IsOffTheRecord() const {
   return false;

components/component_updater/component_updater_service_unittest.cc

@@ -174,6 +174,7 @@ ComponentUpdaterTest::ComponentUpdaterTest()
 
 ComponentUpdaterTest::~ComponentUpdaterTest() {
   EXPECT_CALL(update_client(), RemoveObserver(_)).Times(1);
+  worker_pool_->pool()->Shutdown();
   component_updater_.reset();
 }
 

components/rlz/rlz_tracker_unittest.cc

@@ -8,8 +8,8 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/sequenced_worker_pool_owner.h"
 #include "base/thread_task_runner_handle.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "base/time/time.h"
 #include "components/rlz/rlz_tracker_delegate.h"
 #include "net/url_request/url_request_test_util.h"
@@ -30,10 +30,12 @@ namespace {
 class TestRLZTrackerDelegate : public RLZTrackerDelegate {
  public:
   TestRLZTrackerDelegate()
-      : worker_pool_owner_(1, "TestRLZTracker"),
+      : worker_pool_(new base::SequencedWorkerPool(1, "TestRLZTracker")),
         request_context_getter_(new net::TestURLRequestContextGetter(
             base::ThreadTaskRunnerHandle::Get())) {}
 
+  ~TestRLZTrackerDelegate() override { worker_pool_->Shutdown(); }
+
   void set_brand(const char* brand) { brand_override_ = brand; }
 
   void set_reactivation_brand(const char* reactivation_brand) {
@@ -66,7 +68,7 @@ class TestRLZTrackerDelegate : public RLZTrackerDelegate {
   bool IsOnUIThread() override { return true; }
 
   base::SequencedWorkerPool* GetBlockingPool() override {
-    return worker_pool_owner_.pool().get();
+    return worker_pool_.get();
   }
 
   net::URLRequestContextGetter* GetRequestContext() override {
@@ -106,7 +108,7 @@ class TestRLZTrackerDelegate : public RLZTrackerDelegate {
   }
 
  private:
-  base::SequencedWorkerPoolOwner worker_pool_owner_;
+  scoped_refptr<base::SequencedWorkerPool> worker_pool_;
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
 
   std::string brand_override_;