Fix Thread::SetMessageLoop(nullptr).

Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is to just have a content::TestBrowserThreadBundle member.

This long-standing issue (629139) was now blocking https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being unexpectedly null
is a problem.

Fixing the safe_browsing tests was a bit tricky because the TestSimpleTaskRunner they relied
on doesn't honor delays. Mocking the MessageLoop's TaskRunner with a TestMockTimeTaskRunner
was required. This in turn enabled better tests (that can explicitly wait for the expected timer
to fire). Ideally, their Timers would use the same MockTickClock but FastForwardUntilNoTasksRemain()
being the logical equivalent of the existing TestSimpleTaskRunner::RunUntilIdle() it was used instead
where required to simplify this CL.

BUG=629139, 653916
TBR=danakj (re-enabling DCHECK in thread.cc)

Committed: https://crrev.com/ca6c3690e7a1daaa6b428958dbf247c6611bb7a3
Cr-Commit-Position: refs/heads/master@{#434114}

[gab, 2016-11-10 15:33:01]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 15:33:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-10 15:35:26]

Description was changed from

==========
Fix Thread::SetMessageLoop(nullptr).

BUG=629139, 653916
==========

to

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to constructors need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is often to just have a
content::TestBrowserThreadBundle member.

BUG=629139, 653916
==========

[gab, 2016-11-10 15:37:41]

Description was changed from

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to constructors need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is often to just have a
content::TestBrowserThreadBundle member.

BUG=629139, 653916
==========

to

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is often to just have a
content::TestBrowserThreadBundle member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

BUG=629139, 653916
==========

[commit-bot: I haz the power, 2016-11-10 16:15:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-10 16:15:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-11-22 19:36:30]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 19:36:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-22 19:48:16]

Description was changed from

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is often to just have a
content::TestBrowserThreadBundle member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

BUG=629139, 653916
==========

to

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is to just have a content::TestBrowserThreadBundle
member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

Fixing the safe_browsing tests was a bit tricky because the TestSimpleTaskRunner
they relied
on doesn't honor delays. Mocking the MessageLoop's TaskRunner with a
TestMockTimeTaskRunner
was required. This in turn enabled better tests (that can explicitly wait for
the expected timer
to fire). Ideally, their Timers would use the same MockTickClock but
FastForwardUntilNoTasksRemain() 
being the logical equivalent of the existing
TestSimpleTaskRunner::RunUntilIdle() it was used instead 
where required to simplify this CL.

BUG=629139, 653916
==========

[gab, 2016-11-22 19:54:53]

gab@chromium.org changed reviewers:
+ danakj@chromium.org, mattm@chromium.org

[gab, 2016-11-22 19:54:54]

Matt PTAL at chrome/browser/safe_browsing/* changes.

@Dana, TBR for re-introducing DCHECK in thread.cc and getting one step closer to
fixing http://crbug.com/629139 :)

CC cbentzel FYI per fixing your TODO in protocol_manager_unittest.cc

CC grt for incident_reporting_service.cc unittests.

Thanks,
Gab

https://codereview.chromium.org/2487343005/diff/60001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/2487343005/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc:634:
mock_time_task_runner_->FastForwardUntilNoTasksRemain();
Note: ForwardBy(GetTimeout()) doesn't work here for the same reason as detailed
@ protocol_manager_unittest.cc:645 in this CL.

It would require ForwardBy(GetTimeout() * 2) but this felt weird to drop all
over this file. FastForwardUntilNoTasksRemain() keeps the same logic as before
and follow-up improvements can be done by OWNERS if they wish.

[gab, 2016-11-22 20:08:15]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-11-22 20:58:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 20:58:42]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-11-22 23:50:27]

lgtm

[gab, 2016-11-22 23:55:41]

Description was changed from

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is to just have a content::TestBrowserThreadBundle
member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

Fixing the safe_browsing tests was a bit tricky because the TestSimpleTaskRunner
they relied
on doesn't honor delays. Mocking the MessageLoop's TaskRunner with a
TestMockTimeTaskRunner
was required. This in turn enabled better tests (that can explicitly wait for
the expected timer
to fire). Ideally, their Timers would use the same MockTickClock but
FastForwardUntilNoTasksRemain() 
being the logical equivalent of the existing
TestSimpleTaskRunner::RunUntilIdle() it was used instead 
where required to simplify this CL.

BUG=629139, 653916
==========

to

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is to just have a content::TestBrowserThreadBundle
member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

Fixing the safe_browsing tests was a bit tricky because the TestSimpleTaskRunner
they relied
on doesn't honor delays. Mocking the MessageLoop's TaskRunner with a
TestMockTimeTaskRunner
was required. This in turn enabled better tests (that can explicitly wait for
the expected timer
to fire). Ideally, their Timers would use the same MockTickClock but
FastForwardUntilNoTasksRemain() 
being the logical equivalent of the existing
TestSimpleTaskRunner::RunUntilIdle() it was used instead 
where required to simplify this CL.

BUG=629139, 653916
TBR=danakj (re-enabling DCHECK in thread.cc)
==========

[gab, 2016-11-22 23:56:15]

On 2016/11/22 23:50:27, mattm wrote:
> lgtm

Thanks, TBR Dana for re-enabling DCHECK in thread.cc

[gab, 2016-11-22 23:56:22]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-11-22 23:57:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 00:46:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 00:46:48]

Failed to apply patch for
chrome/browser/safe_browsing/protocol_manager_unittest.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/safe_browsing/protocol_manager_unittest.cc:9
  error: chrome/browser/safe_browsing/protocol_manager_unittest.cc: patch does
not apply

Patch:       chrome/browser/safe_browsing/protocol_manager_unittest.cc
Index: chrome/browser/safe_browsing/protocol_manager_unittest.cc
diff --git a/chrome/browser/safe_browsing/protocol_manager_unittest.cc
b/chrome/browser/safe_browsing/protocol_manager_unittest.cc
index
a07df56a650151e03d8db85c30ece6e69c4ffa65..702758c6032e7d2bc30400afa076c7e004336672
100644
--- a/chrome/browser/safe_browsing/protocol_manager_unittest.cc
+++ b/chrome/browser/safe_browsing/protocol_manager_unittest.cc
@@ -9,13 +9,13 @@
 
 #include "base/message_loop/message_loop.h"
 #include "base/strings/stringprintf.h"
-#include "base/test/test_simple_task_runner.h"
+#include "base/test/scoped_mock_time_message_loop_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/chunk.pb.h"
 #include "components/safe_browsing_db/safebrowsing.pb.h"
 #include "components/safe_browsing_db/util.h"
-#include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
@@ -73,9 +73,8 @@ namespace safe_browsing {
 class SafeBrowsingProtocolManagerTest : public testing::Test {
  protected:
   SafeBrowsingProtocolManagerTest()
-      : runner_(new base::TestSimpleTaskRunner),
-        runner_handler_(runner_),
-        io_thread_(content::BrowserThread::IO, base::MessageLoop::current()) {}
+      : thread_bundle_(content::TestBrowserThreadBundle::Options::IO_MAINLOOP)
{
+  }
 
   ~SafeBrowsingProtocolManagerTest() override {}
 
@@ -129,9 +128,13 @@ class SafeBrowsingProtocolManagerTest : public
testing::Test {
     EXPECT_EQ(GURL(expected_url), url_fetcher->GetOriginalURL());
   }
 
-  scoped_refptr<base::TestSimpleTaskRunner> runner_;
-  base::ThreadTaskRunnerHandle runner_handler_;
-  content::TestBrowserThread io_thread_;
+  // Fakes BrowserThreads and the main MessageLoop.
+  content::TestBrowserThreadBundle thread_bundle_;
+
+  // Replaces the main MessageLoop's TaskRunner with a TaskRunner on which time
+  // is mocked to allow testing of things bound to timers below.
+  base::ScopedMockTimeMessageLoopTaskRunner mock_time_task_runner_;
+
   std::string key_param_;
 };
 
@@ -411,7 +414,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
ProblemAccessingDatabase) {
       CreateProtocolManager(&test_delegate));
 
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   EXPECT_TRUE(pm->IsUpdateScheduled());
 }
@@ -446,7 +449,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, ExistingDatabase) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -486,7 +489,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseBadBodyBackupSuccess) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -530,7 +533,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpErrorBackupError) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -574,7 +577,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpErrorBackupSuccess) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -618,7 +621,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpErrorBackupTimeout) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -635,15 +638,21 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpErrorBackupTimeout) {
       url_fetcher_factory.GetFetcherByID(1);
   ValidateUpdateFetcherRequest(backup_url_fetcher, kBackupHttpUrlPrefix, "");
 
-  // Either one or two calls to RunPendingTasks are needed here. The first run
-  // of RunPendingTasks will run the canceled timeout task associated with
-  // the first Update request. Depending on timing, this will either directly
-  // call the timeout task from the backup request, or schedule another task
-  // to run that in the future.
-  // TODO(cbentzel): Less fragile approach.
-  runner_->RunPendingTasks();
-  if (!pm->IsUpdateScheduled())
-    runner_->RunPendingTasks();
+  // Confirm that no update is scheduled (still waiting on a response to the
+  // backup request).
+  EXPECT_FALSE(pm->IsUpdateScheduled());
+
+  // Force the timeout to fire. Need to fast forward by twice the timeout
amount
+  // as issuing the backup request above restarted the timeout timer but that
+  // Timer's clock isn't mocked and its impl is such that it will re-use its
+  // initial delayed task and re-post by the remainder of the timeout when it
+  // fires (which is pretty much the full timeout in real time since we mock
the
+  // wait). A cleaner solution would be to pass
+  // |mock_time_task_runner_->GetMockTickClock()| to the
+  // SafeBrowsingProtocolManager's Timers but such hooks were deemed overkill
+  // per this being the only use case at this point.
+  mock_time_task_runner_->FastForwardBy(
+      SafeBrowsingProtocolManager::GetUpdateTimeoutForTesting() * 2);
   EXPECT_TRUE(pm->IsUpdateScheduled());
 }
 
@@ -666,7 +675,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -710,7 +719,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -753,7 +762,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -798,7 +807,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -841,15 +850,15 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseTimeoutBackupSuccess) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
   ValidateUpdateFetcherRequest(url_fetcher);
 
-  // The first time RunPendingTasks is called above, the update timeout timer
is
-  // not handled. This call of RunPendingTasks will handle the update.
-  runner_->RunPendingTasks();
+  // Force the timeout to fire.
+  mock_time_task_runner_->FastForwardBy(
+      SafeBrowsingProtocolManager::GetUpdateTimeoutForTesting());
 
   // There should be a backup URLFetcher now.
   net::TestURLFetcher* backup_url_fetcher =
@@ -883,7 +892,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, UpdateResponseReset)
{
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
   ValidateUpdateFetcherRequest(url_fetcher);
@@ -915,7 +924,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
EmptyRedirectResponse) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // The update response contains a single redirect command.
   net::TestURLFetcher*…
(message too large)

[gab, 2016-11-23 02:13:30]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-11-23 02:13:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 02:23:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 02:23:19]

Failed to apply patch for
chrome/browser/safe_browsing/protocol_manager_unittest.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/safe_browsing/protocol_manager_unittest.cc:9
  error: chrome/browser/safe_browsing/protocol_manager_unittest.cc: patch does
not apply

Patch:       chrome/browser/safe_browsing/protocol_manager_unittest.cc
Index: chrome/browser/safe_browsing/protocol_manager_unittest.cc
diff --git a/chrome/browser/safe_browsing/protocol_manager_unittest.cc
b/chrome/browser/safe_browsing/protocol_manager_unittest.cc
index
a07df56a650151e03d8db85c30ece6e69c4ffa65..702758c6032e7d2bc30400afa076c7e004336672
100644
--- a/chrome/browser/safe_browsing/protocol_manager_unittest.cc
+++ b/chrome/browser/safe_browsing/protocol_manager_unittest.cc
@@ -9,13 +9,13 @@
 
 #include "base/message_loop/message_loop.h"
 #include "base/strings/stringprintf.h"
-#include "base/test/test_simple_task_runner.h"
+#include "base/test/scoped_mock_time_message_loop_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/chunk.pb.h"
 #include "components/safe_browsing_db/safebrowsing.pb.h"
 #include "components/safe_browsing_db/util.h"
-#include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
@@ -73,9 +73,8 @@ namespace safe_browsing {
 class SafeBrowsingProtocolManagerTest : public testing::Test {
  protected:
   SafeBrowsingProtocolManagerTest()
-      : runner_(new base::TestSimpleTaskRunner),
-        runner_handler_(runner_),
-        io_thread_(content::BrowserThread::IO, base::MessageLoop::current()) {}
+      : thread_bundle_(content::TestBrowserThreadBundle::Options::IO_MAINLOOP)
{
+  }
 
   ~SafeBrowsingProtocolManagerTest() override {}
 
@@ -129,9 +128,13 @@ class SafeBrowsingProtocolManagerTest : public
testing::Test {
     EXPECT_EQ(GURL(expected_url), url_fetcher->GetOriginalURL());
   }
 
-  scoped_refptr<base::TestSimpleTaskRunner> runner_;
-  base::ThreadTaskRunnerHandle runner_handler_;
-  content::TestBrowserThread io_thread_;
+  // Fakes BrowserThreads and the main MessageLoop.
+  content::TestBrowserThreadBundle thread_bundle_;
+
+  // Replaces the main MessageLoop's TaskRunner with a TaskRunner on which time
+  // is mocked to allow testing of things bound to timers below.
+  base::ScopedMockTimeMessageLoopTaskRunner mock_time_task_runner_;
+
   std::string key_param_;
 };
 
@@ -411,7 +414,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
ProblemAccessingDatabase) {
       CreateProtocolManager(&test_delegate));
 
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   EXPECT_TRUE(pm->IsUpdateScheduled());
 }
@@ -446,7 +449,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, ExistingDatabase) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -486,7 +489,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseBadBodyBackupSuccess) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -530,7 +533,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpErrorBackupError) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -574,7 +577,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpErrorBackupSuccess) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -618,7 +621,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpErrorBackupTimeout) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -635,15 +638,21 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseHttpErrorBackupTimeout) {
       url_fetcher_factory.GetFetcherByID(1);
   ValidateUpdateFetcherRequest(backup_url_fetcher, kBackupHttpUrlPrefix, "");
 
-  // Either one or two calls to RunPendingTasks are needed here. The first run
-  // of RunPendingTasks will run the canceled timeout task associated with
-  // the first Update request. Depending on timing, this will either directly
-  // call the timeout task from the backup request, or schedule another task
-  // to run that in the future.
-  // TODO(cbentzel): Less fragile approach.
-  runner_->RunPendingTasks();
-  if (!pm->IsUpdateScheduled())
-    runner_->RunPendingTasks();
+  // Confirm that no update is scheduled (still waiting on a response to the
+  // backup request).
+  EXPECT_FALSE(pm->IsUpdateScheduled());
+
+  // Force the timeout to fire. Need to fast forward by twice the timeout
amount
+  // as issuing the backup request above restarted the timeout timer but that
+  // Timer's clock isn't mocked and its impl is such that it will re-use its
+  // initial delayed task and re-post by the remainder of the timeout when it
+  // fires (which is pretty much the full timeout in real time since we mock
the
+  // wait). A cleaner solution would be to pass
+  // |mock_time_task_runner_->GetMockTickClock()| to the
+  // SafeBrowsingProtocolManager's Timers but such hooks were deemed overkill
+  // per this being the only use case at this point.
+  mock_time_task_runner_->FastForwardBy(
+      SafeBrowsingProtocolManager::GetUpdateTimeoutForTesting() * 2);
   EXPECT_TRUE(pm->IsUpdateScheduled());
 }
 
@@ -666,7 +675,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -710,7 +719,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -753,7 +762,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -798,7 +807,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
@@ -841,15 +850,15 @@ TEST_F(SafeBrowsingProtocolManagerTest,
UpdateResponseTimeoutBackupSuccess) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // We should have an URLFetcher at this point in time.
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
   ValidateUpdateFetcherRequest(url_fetcher);
 
-  // The first time RunPendingTasks is called above, the update timeout timer
is
-  // not handled. This call of RunPendingTasks will handle the update.
-  runner_->RunPendingTasks();
+  // Force the timeout to fire.
+  mock_time_task_runner_->FastForwardBy(
+      SafeBrowsingProtocolManager::GetUpdateTimeoutForTesting());
 
   // There should be a backup URLFetcher now.
   net::TestURLFetcher* backup_url_fetcher =
@@ -883,7 +892,7 @@ TEST_F(SafeBrowsingProtocolManagerTest, UpdateResponseReset)
{
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   net::TestURLFetcher* url_fetcher = url_fetcher_factory.GetFetcherByID(0);
   ValidateUpdateFetcherRequest(url_fetcher);
@@ -915,7 +924,7 @@ TEST_F(SafeBrowsingProtocolManagerTest,
EmptyRedirectResponse) {
 
   // Kick off initialization. This returns chunks from the DB synchronously.
   pm->ForceScheduleNextUpdate(TimeDelta());
-  runner_->RunPendingTasks();
+  mock_time_task_runner_->RunUntilIdle();
 
   // The update response contains a single redirect command.
   net::TestURLFetcher*…
(message too large)

[gab, 2016-11-23 03:06:43]

The CQ bit was checked by gab@chromium.org

[gab, 2016-11-23 03:06:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2487343005/#ps80001
(title: "Merge up to r434093")

[commit-bot: I haz the power, 2016-11-23 03:06:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 04:25:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479870403475390,
"parent_rev": "9b42e8dcd7b3f87467d21fe40f189388de2f653f", "commit_rev":
"b54fa47df94c5e7596dd330340bace886293ddbc"}

[commit-bot: I haz the power, 2016-11-23 04:25:56]

Description was changed from

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is to just have a content::TestBrowserThreadBundle
member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

Fixing the safe_browsing tests was a bit tricky because the TestSimpleTaskRunner
they relied
on doesn't honor delays. Mocking the MessageLoop's TaskRunner with a
TestMockTimeTaskRunner
was required. This in turn enabled better tests (that can explicitly wait for
the expected timer
to fire). Ideally, their Timers would use the same MockTickClock but
FastForwardUntilNoTasksRemain() 
being the logical equivalent of the existing
TestSimpleTaskRunner::RunUntilIdle() it was used instead 
where required to simplify this CL.

BUG=629139, 653916
TBR=danakj (re-enabling DCHECK in thread.cc)
==========

to

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is to just have a content::TestBrowserThreadBundle
member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

Fixing the safe_browsing tests was a bit tricky because the TestSimpleTaskRunner
they relied
on doesn't honor delays. Mocking the MessageLoop's TaskRunner with a
TestMockTimeTaskRunner
was required. This in turn enabled better tests (that can explicitly wait for
the expected timer
to fire). Ideally, their Timers would use the same MockTickClock but
FastForwardUntilNoTasksRemain() 
being the logical equivalent of the existing
TestSimpleTaskRunner::RunUntilIdle() it was used instead 
where required to simplify this CL.

BUG=629139, 653916
TBR=danakj (re-enabling DCHECK in thread.cc)
==========

[commit-bot: I haz the power, 2016-11-23 04:25:57]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-11-23 04:27:47]

Description was changed from

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is to just have a content::TestBrowserThreadBundle
member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

Fixing the safe_browsing tests was a bit tricky because the TestSimpleTaskRunner
they relied
on doesn't honor delays. Mocking the MessageLoop's TaskRunner with a
TestMockTimeTaskRunner
was required. This in turn enabled better tests (that can explicitly wait for
the expected timer
to fire). Ideally, their Timers would use the same MockTickClock but
FastForwardUntilNoTasksRemain() 
being the logical equivalent of the existing
TestSimpleTaskRunner::RunUntilIdle() it was used instead 
where required to simplify this CL.

BUG=629139, 653916
TBR=danakj (re-enabling DCHECK in thread.cc)
==========

to

==========
Fix Thread::SetMessageLoop(nullptr).

Unit tests that pass base::MessageLoop::current() to a Thread need to
explicitly have a base::MessageLoop member as well or current() is null.

A better alternative though is to just have a content::TestBrowserThreadBundle
member.

This long-standing issue (629139) was now blocking
https://codereview.chromium.org/2464233002
which needs to unconditionally inspect the MessageLoop's member and it being
unexpectedly null
is a problem.

Fixing the safe_browsing tests was a bit tricky because the TestSimpleTaskRunner
they relied
on doesn't honor delays. Mocking the MessageLoop's TaskRunner with a
TestMockTimeTaskRunner
was required. This in turn enabled better tests (that can explicitly wait for
the expected timer
to fire). Ideally, their Timers would use the same MockTickClock but
FastForwardUntilNoTasksRemain()
being the logical equivalent of the existing
TestSimpleTaskRunner::RunUntilIdle() it was used instead
where required to simplify this CL.

BUG=629139, 653916
TBR=danakj (re-enabling DCHECK in thread.cc)

Committed: https://crrev.com/ca6c3690e7a1daaa6b428958dbf247c6611bb7a3
Cr-Commit-Position: refs/heads/master@{#434114}
==========

[commit-bot: I haz the power, 2016-11-23 04:27:49]

Patchset 4 (id:??) landed as
https://crrev.com/ca6c3690e7a1daaa6b428958dbf247c6611bb7a3
Cr-Commit-Position: refs/heads/master@{#434114}

[danakj, 2016-11-29 01:51:06]

LGTM