Remove some usage of base::MessageLoopCurrent::SetTaskRunner() in tests.

This is an expanded and leak-fixed reland of crrev.com/c/1177641 and
crrev.com/c/1176894, though it tackles the root of the problem rather than
working around it.

base::MessageLoopCurrent::SetTaskRunner() is deprecated since only owners of
the MessageLoop instance should replace its TaskRunner. Odd things happen in
tests when the message loop task runner is replaced and a profile is
instantiated. This CL removes some more usage of the method in tests where
there is interaction with a TestingProfile object.

BUG=616447

Change-Id: Ic2ad6eb2a8699f103e432af74ad9c71c1dd43656
Reviewed-on: https://chromium-review.googlesource.com/1179111
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584249}

chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.h"
 
 #include <string>
+#include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -78,21 +79,27 @@ void SAMLOfflineSigninLimiter::SignedIn(UserContext::AuthFlow auth_flow) {
   UpdateLimit();
 }
 
+void SAMLOfflineSigninLimiter::SetTimerForTesting(
+    std::unique_ptr<base::OneShotTimer> timer) {
+  offline_signin_limit_timer_ = std::move(timer);
+}
+
 void SAMLOfflineSigninLimiter::Shutdown() {
+  offline_signin_limit_timer_->Stop();
   pref_change_registrar_.RemoveAll();
-  offline_signin_limit_timer_.reset();
 }
 
 SAMLOfflineSigninLimiter::SAMLOfflineSigninLimiter(Profile* profile,
                                                    base::Clock* clock)
     : profile_(profile),
-      clock_(clock ? clock : base::DefaultClock::GetInstance()) {}
+      clock_(clock ? clock : base::DefaultClock::GetInstance()),
+      offline_signin_limit_timer_(std::make_unique<base::OneShotTimer>()) {}
 
 SAMLOfflineSigninLimiter::~SAMLOfflineSigninLimiter() {}
 
 void SAMLOfflineSigninLimiter::UpdateLimit() {
   // Stop the |offline_signin_limit_timer_|.
-  offline_signin_limit_timer_.reset();
+  offline_signin_limit_timer_->Stop();
 
   PrefService* prefs = pref_change_registrar_.prefs();
   const base::TimeDelta offline_signin_time_limit =
@@ -126,7 +133,6 @@ void SAMLOfflineSigninLimiter::UpdateLimit() {
 
   // Arm |offline_signin_limit_timer_| so that it sets the flag enforcing online
   // login when the limit expires.
-  offline_signin_limit_timer_.reset(new base::OneShotTimer);
   offline_signin_limit_timer_->Start(
       FROM_HERE, offline_signin_time_limit - time_since_last_gaia_signin, this,
       &SAMLOfflineSigninLimiter::ForceOnlineLogin);
@@ -143,7 +149,7 @@ void SAMLOfflineSigninLimiter::ForceOnlineLogin() {
   user_manager::UserManager::Get()->SaveForceOnlineSignin(user->GetAccountId(),
                                                           true);
   RecordReauthReason(user->GetAccountId(), ReauthReason::SAML_REAUTH_POLICY);
-  offline_signin_limit_timer_.reset();
+  offline_signin_limit_timer_->Stop();
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.h

@@ -38,6 +38,9 @@ class SAMLOfflineSigninLimiter : public KeyedService {
   // the type of authentication flow that the user went through.
   void SignedIn(UserContext::AuthFlow auth_flow);
 
+  // Allows a mock timer to be substituted for testing purposes.
+  void SetTimerForTesting(std::unique_ptr<base::OneShotTimer> timer);
+
   // KeyedService:
   void Shutdown() override;
 

chrome/browser/chromeos/login/saml/saml_offline_signin_limiter_unittest.cc

@@ -4,13 +4,14 @@
 
 #include "chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
-#include "base/memory/ref_counted.h"
-#include "base/message_loop/message_loop_current.h"
 #include "base/test/simple_test_clock.h"
-#include "base/test/test_simple_task_runner.h"
 #include "base/time/clock.h"
+#include "base/timer/mock_timer.h"
 #include "chrome/browser/chromeos/login/saml/saml_offline_signin_limiter_factory.h"
 #include "chrome/browser/chromeos/login/users/mock_user_manager.h"
 #include "chrome/browser/profiles/profile.h"
@@ -26,10 +27,10 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
 using testing::Mock;
 using testing::Return;
 using testing::Sequence;
-using testing::_;
 
 namespace chromeos {
 
@@ -59,13 +60,12 @@ class SAMLOfflineSigninLimiterTest : public testing::Test {
   extensions::QuotaService::ScopedDisablePurgeForTesting
       disable_purge_for_testing_;
 
-  scoped_refptr<base::TestSimpleTaskRunner> runner_;
-
   MockUserManager* user_manager_;  // Not owned.
   user_manager::ScopedUserManager user_manager_enabler_;
 
   std::unique_ptr<TestingProfile> profile_;
   base::SimpleTestClock clock_;
+  base::MockOneShotTimer* timer_;  // Not owned.
 
   SAMLOfflineSigninLimiter* limiter_;  // Owned.
 
@@ -75,10 +75,10 @@ class SAMLOfflineSigninLimiterTest : public testing::Test {
 };
 
 SAMLOfflineSigninLimiterTest::SAMLOfflineSigninLimiterTest()
-    : runner_(new base::TestSimpleTaskRunner),
-      user_manager_(new MockUserManager),
+    : user_manager_(new MockUserManager),
       user_manager_enabler_(base::WrapUnique(user_manager_)),
-      limiter_(NULL) {}
+      timer_(nullptr),
+      limiter_(nullptr) {}
 
 SAMLOfflineSigninLimiterTest::~SAMLOfflineSigninLimiterTest() {
   DestroyLimiter();
@@ -93,13 +93,17 @@ void SAMLOfflineSigninLimiterTest::DestroyLimiter() {
   if (limiter_) {
     limiter_->Shutdown();
     delete limiter_;
-    limiter_ = NULL;
+    limiter_ = nullptr;
+    timer_ = nullptr;
   }
 }
 
 void SAMLOfflineSigninLimiterTest::CreateLimiter() {
   DestroyLimiter();
   limiter_ = new SAMLOfflineSigninLimiter(profile_.get(), &clock_);
+  auto timer = std::make_unique<base::MockOneShotTimer>();
+  timer_ = timer.get();
+  limiter_->SetTimerForTesting(std::move(timer));
 }
 
 void SAMLOfflineSigninLimiterTest::SetUpUserManager() {
@@ -108,7 +112,6 @@ void SAMLOfflineSigninLimiterTest::SetUpUserManager() {
 }
 
 void SAMLOfflineSigninLimiterTest::SetUp() {
-  base::MessageLoopCurrent::Get()->SetTaskRunner(runner_);
   profile_.reset(new TestingProfile);
 
   SAMLOfflineSigninLimiterFactory::SetClockForTesting(&clock_);
@@ -125,7 +128,7 @@ TestingPrefServiceSimple* SAMLOfflineSigninLimiterTest::GetTestingLocalState() {
 }
 
 void SAMLOfflineSigninLimiterTest::TearDown() {
-  SAMLOfflineSigninLimiterFactory::SetClockForTesting(NULL);
+  SAMLOfflineSigninLimiterFactory::SetClockForTesting(nullptr);
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLDefaultLimit) {
@@ -150,8 +153,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLDefaultLimit) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
-
+  EXPECT_FALSE(timer_->IsRunning());
   // Log out. Verify that the flag enforcing online login is not set.
   DestroyLimiter();
 
@@ -171,7 +173,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLDefaultLimit) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLNoLimit) {
@@ -199,7 +201,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLNoLimit) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 
   // Log out. Verify that the flag enforcing online login is not set.
   DestroyLimiter();
@@ -220,7 +222,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLNoLimit) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLZeroLimit) {
@@ -248,7 +250,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLZeroLimit) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 
   // Log out. Verify that the flag enforcing online login is not set.
   DestroyLimiter();
@@ -269,7 +271,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLZeroLimit) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLSetLimitWhileLoggedIn) {
@@ -297,13 +299,13 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLSetLimitWhileLoggedIn) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 
   // Set a zero time limit.
   prefs->SetInteger(prefs::kSAMLOfflineSigninTimeLimit, 0);
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLRemoveLimitWhileLoggedIn) {
@@ -328,13 +330,13 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLRemoveLimitWhileLoggedIn) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 
   // Remove the time limit.
   prefs->SetInteger(prefs::kSAMLOfflineSigninTimeLimit, -1);
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLLogInWithExpiredLimit) {
@@ -362,7 +364,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, NoSAMLLogInWithExpiredLimit) {
   EXPECT_FALSE(pref->HasUserSetting());
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, SAMLDefaultLimit) {
@@ -382,7 +384,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLDefaultLimit) {
   EXPECT_EQ(clock_.Now(), last_gaia_signin_time);
 
   // Verify that the timer is running.
-  EXPECT_TRUE(runner_->HasPendingTask());
+  EXPECT_TRUE(timer_->IsRunning());
 
   // Log out. Verify that the flag enforcing online login is not set.
   DestroyLimiter();
@@ -406,7 +408,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLDefaultLimit) {
   EXPECT_EQ(clock_.Now(), last_gaia_signin_time);
 
   // Verify that the timer is running.
-  EXPECT_TRUE(runner_->HasPendingTask());
+  EXPECT_TRUE(timer_->IsRunning());
 
   // Log out. Verify that the flag enforcing online login is not set.
   DestroyLimiter();
@@ -431,7 +433,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLDefaultLimit) {
   EXPECT_EQ(gaia_signin_time, last_gaia_signin_time);
 
   // Verify that the timer is running.
-  EXPECT_TRUE(runner_->HasPendingTask());
+  EXPECT_TRUE(timer_->IsRunning());
 
   // Advance time by four weeks.
   clock_.Advance(base::TimeDelta::FromDays(28));  // 4 weeks.
@@ -444,7 +446,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLDefaultLimit) {
       .Times(0);
   EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(test_account_id_, true))
       .Times(1);
-  runner_->RunPendingTasks();
+  timer_->Fire();
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, SAMLNoLimit) {
@@ -467,7 +469,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLNoLimit) {
   EXPECT_EQ(clock_.Now(), last_gaia_signin_time);
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 
   // Log out. Verify that the flag enforcing online login is not set.
   DestroyLimiter();
@@ -491,7 +493,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLNoLimit) {
   EXPECT_EQ(clock_.Now(), last_gaia_signin_time);
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 
   // Log out. Verify that the flag enforcing online login is not set.
   DestroyLimiter();
@@ -516,7 +518,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLNoLimit) {
   EXPECT_EQ(gaia_signin_time, last_gaia_signin_time);
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, SAMLZeroLimit) {
@@ -563,7 +565,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLSetLimitWhileLoggedIn) {
   EXPECT_EQ(clock_.Now(), last_gaia_signin_time);
 
   // Verify that no timer is running.
-  EXPECT_FALSE(runner_->HasPendingTask());
+  EXPECT_FALSE(timer_->IsRunning());
 
   // Set a zero time limit. Verify that the flag enforcing online login is set.
   Mock::VerifyAndClearExpectations(user_manager_);
@@ -592,7 +594,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLRemoveLimit) {
   EXPECT_EQ(clock_.Now(), last_gaia_signin_time);
 
   // Verify that the timer is running.
-  EXPECT_TRUE(runner_->HasPendingTask());
+  EXPECT_TRUE(timer_->IsRunning());
 
   // Remove the time limit.
   prefs->SetInteger(prefs::kSAMLOfflineSigninTimeLimit, -1);
@@ -605,7 +607,6 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLRemoveLimit) {
       .Times(0);
   EXPECT_CALL(*user_manager_, SaveForceOnlineSignin(test_account_id_, true))
       .Times(0);
-  runner_->RunUntilIdle();
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, SAMLLogInWithExpiredLimit) {
@@ -632,7 +633,7 @@ TEST_F(SAMLOfflineSigninLimiterTest, SAMLLogInWithExpiredLimit) {
   EXPECT_EQ(clock_.Now(), last_gaia_signin_time);
 
   // Verify that the timer is running.
-  EXPECT_TRUE(runner_->HasPendingTask());
+  EXPECT_TRUE(timer_->IsRunning());
 }
 
 TEST_F(SAMLOfflineSigninLimiterTest, SAMLLogInOfflineWithExpiredLimit) {

chrome/browser/safe_browsing/incident_reporting/platform_state_store_win_unittest.cc

@@ -7,10 +7,8 @@
 #include <utility>
 
 #include "base/macros.h"
-#include "base/message_loop/message_loop_current.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_reg_util_win.h"
-#include "base/test/test_simple_task_runner.h"
 #include "base/win/registry.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/test/base/testing_browser_process.h"
@@ -37,12 +35,10 @@ class PlatformStateStoreWinTest : public ::testing::Test {
  protected:
   PlatformStateStoreWinTest()
       : profile_(nullptr),
-        task_runner_(new base::TestSimpleTaskRunner()),
         profile_manager_(TestingBrowserProcess::GetGlobal()) {}
 
   void SetUp() override {
     ::testing::Test::SetUp();
-    base::MessageLoopCurrent::Get()->SetTaskRunner(task_runner_);
     ASSERT_NO_FATAL_FAILURE(
         registry_override_manager_.OverrideRegistry(HKEY_CURRENT_USER));
     ASSERT_TRUE(profile_manager_.SetUp());
@@ -118,7 +114,6 @@ class PlatformStateStoreWinTest : public ::testing::Test {
  private:
   content::TestBrowserThreadBundle thread_bundle_;
   registry_util::RegistryOverrideManager registry_override_manager_;
-  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   TestingProfileManager profile_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(PlatformStateStoreWinTest);

chrome/browser/safe_browsing/incident_reporting/state_store_unittest.cc

@@ -10,10 +10,8 @@
 #include "base/json/json_file_value_serializer.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/message_loop/message_loop_current.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_simple_task_runner.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/prefs/browser_prefs.h"
@@ -80,7 +78,6 @@ class StateStoreTest : public PlatformStateStoreTestBase {
 
   void SetUp() override {
     PlatformStateStoreTestBase::SetUp();
-    base::MessageLoopCurrent::Get()->SetTaskRunner(task_runner_);
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     ASSERT_TRUE(profile_manager_.SetUp());
     CreateProfile();
@@ -93,6 +90,14 @@ class StateStoreTest : public PlatformStateStoreTestBase {
     }
   }
 
+  // Commits a pending write (which posts a task to task_runner_) and waits for
+  // it to finish.
+  void CommitWrite() {
+    ASSERT_NE(nullptr, profile_);
+    profile_->GetPrefs()->CommitPendingWrite();
+    task_runner_->RunUntilIdle();
+  }
+
   // Removes the safebrowsing.incidents_sent preference from the profile's pref
   // store.
   void TrimPref() {
@@ -208,8 +213,8 @@ TEST_F(StateStoreTest, ClearAll) {
     ASSERT_FALSE(state_store.HasBeenReported(data.type, data.key, data.digest));
   }
 
-  // Run tasks to write prefs out to the JsonPrefStore.
-  task_runner_->RunUntilIdle();
+  // Write prefs out to the JsonPrefStore.
+  CommitWrite();
 
   // Delete the profile.
   DeleteProfile();
@@ -234,7 +239,7 @@ TEST_F(StateStoreTest, Persistence) {
   }
 
   // Run tasks to write prefs out to the JsonPrefStore.
-  task_runner_->RunUntilIdle();
+  CommitWrite();
 
   // Delete the profile.
   DeleteProfile();
@@ -257,8 +262,8 @@ TEST_F(StateStoreTest, PersistenceWithStoreDelete) {
       transaction.MarkAsReported(data.type, data.key, data.digest);
   }
 
-  // Run tasks to write prefs out to the JsonPrefStore.
-  task_runner_->RunUntilIdle();
+  // Write prefs out to the JsonPrefStore.
+  CommitWrite();
 
   // Delete the profile.
   DeleteProfile();