Ensure tests have an active task runner

Ensure tests have an active task runner

This patch modifies all tests to have a more realistic threading set-up by either:

  a) adding a TestBrowserThreadBundle
  b) constructing any existing TestBrowserThreadBundle earlier or
  c) adding a MessageLoop as appropriate.

This makes it possible to later migrate base/ from MessageLoopProxy over to
ThreadTaskRunnerHandle, which requires an active task runner on the current thread.

BUG=465354
Committed: https://crrev.com/0becb33779f8678e2703d6ff1a73ce2b45571f98
Cr-Commit-Position: refs/heads/master@{#327067}

[Sami, 2015-04-17 14:21:47]

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

[commit-bot: I haz the power, 2015-04-17 14:22:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1086733002/390002

[commit-bot: I haz the power, 2015-04-17 16:05:35]

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

[commit-bot: I haz the power, 2015-04-17 16:05:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Sami, 2015-04-17 17:59:27]

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

[commit-bot: I haz the power, 2015-04-17 18:00:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1086733002/420001

[commit-bot: I haz the power, 2015-04-17 19:09:56]

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

[commit-bot: I haz the power, 2015-04-17 19:09:58]

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

[Sami, 2015-04-20 19:04:15]

skyostil@chromium.org changed reviewers:
+ jam@chromium.org

[Sami, 2015-04-20 19:07:08]

Sorry for the ginormous patch, but I wanted to keep the actual change in base/
and the test fixes together and it's all mostly mechanical anyway.

[Sami, 2015-04-20 19:08:27]

skyostil@chromium.org changed reviewers:
+ atwilson@chromium.org

[Sami, 2015-04-20 19:11:36]

Andrew, could you have a look at
chrome/browser/background/background_mode_manager_unittest.cc since its the only
test I had to modify in a substantial way? Basically I couldn't find a way to
make BackgroundModeManagerTest and BackgroundModeManagerWithExtensionsTest work
with the same threading setup so I split them up.

[jam, 2015-04-21 15:31:47]

I started reviewing this but this is too big. is there no way you can split it
up?

if you can't, please find more reviewers to lighten the load. i.e. base/ should
be reviewed by a base owner. look for other owners in components, net etc to
review their respective directories.

[Sami, 2015-04-21 18:48:02]

On 2015/04/21 15:31:47, jam wrote:
> I started reviewing this but this is too big. is there no way you can split it
> up?
>
> if you can't, please find more reviewers to lighten the load. i.e. base/
should
> be reviewed by a base owner. look for other owners in components, net etc to
> review their respective directories.

Fair point. I've now reduced this to just the test changes and will upload
another patch for the functional changes in base/.

Does it look reasonably sized now? If not, I'd be happy to track down more
reviewers.

Just as a summary, besides the TestBrowserThreadBundle/MessageLoop oneliners,
the main changes are:

1. chrome/browser/background/background_mode_manager_unittest.cc (on which I'm
hoping to hear from atwilson@)
2. BrowserThreadImpl::PostTaskHelper won't initialize a new globals pool.
3. Simple(Cache)BackendImpl won't flush the worker pool for testing if no task
runner exists.

[Andrew T Wilson (Slow), 2015-04-22 14:48:03]

https://codereview.chromium.org/1086733002/diff/500001/chrome/browser/backgro...
File chrome/browser/background/background_mode_manager_unittest.cc (right):

https://codereview.chromium.org/1086733002/diff/500001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager_unittest.cc:170: // to avoid
having the browser's shutdown logic try to access a non-existent
I guess I'm not sure I understand - are you saying that there are shutdown tasks
that this dummy_task_runner_ won't execute? I'm not 100% clear why this class
gets a dummy_task_runner_ but BackgroundModeManagerWithExtensionTest does not?

[jam, 2015-04-22 15:41:18]

Thanks for reducing it, this is more manageable.

my only comment is i don't understand why the browserthread change is needed?

On 2015/04/21 18:48:02, Sami wrote:
> On 2015/04/21 15:31:47, jam wrote:
> > I started reviewing this but this is too big. is there no way you can split
it
> > up?
> >
> > if you can't, please find more reviewers to lighten the load. i.e. base/
> should
> > be reviewed by a base owner. look for other owners in components, net etc to
> > review their respective directories.
> 
> Fair point. I've now reduced this to just the test changes and will upload
> another patch for the functional changes in base/.
> 
> Does it look reasonably sized now? If not, I'd be happy to track down more
> reviewers.
> 
> Just as a summary, besides the TestBrowserThreadBundle/MessageLoop oneliners,
> the main changes are:
> 
> 1. chrome/browser/background/background_mode_manager_unittest.cc (on which I'm
> hoping to hear from atwilson@)
> 2. BrowserThreadImpl::PostTaskHelper won't initialize a new globals pool.
> 3. Simple(Cache)BackendImpl won't flush the worker pool for testing if no task
> runner exists.

[Sami, 2015-04-22 18:06:47]

Thanks for having a look.

On 2015/04/22 15:41:18, jam wrote:
> my only comment is i don't understand why the browserthread change is needed?

It was motivated by this DCHECK failure during test teardown:

[21975:21975:0422/182840:2544456752510:FATAL:thread_task_runner_handle.cc(23)]
Check failed: current. 
#0 0x7fa04653bc8e base::debug::StackTrace::StackTrace()
#1 0x7fa04658e22f logging::LogMessage::~LogMessage()
#2 0x7fa04666c067 base::ThreadTaskRunnerHandle::Get()
#3 0x7fa046671d9a base::SequencedWorkerPool::SequencedWorkerPool()
#4 0x7fa04f3de89b content::(anonymous
namespace)::BrowserThreadGlobals::BrowserThreadGlobals()
#5 0x7fa04f3de81a base::DefaultLazyInstanceTraits<>::New()
#6 0x7fa04f3de745 base::internal::LeakyLazyInstanceTraits<>::New()
#7 0x7fa04f3de6fd base::LazyInstance<>::Pointer()
#8 0x7fa04f3dcbd5 base::LazyInstance<>::Get()
#9 0x7fa04f3dd896 content::BrowserThreadImpl::PostTaskHelper()
#10 0x7fa04f3de276 content::BrowserThread::PostNonNestableDelayedTask()
#11 0x7fa04f3deca8 content::(anonymous
namespace)::BrowserThreadMessageLoopProxy::PostNonNestableDelayedTask()
#12 0x7fa046611ab9 base::SequencedTaskRunner::PostNonNestableTask()
#13 0x7fa046611b23 base::SequencedTaskRunner::DeleteSoonInternal()
#14 0x7fa0452c3a27
base::subtle::DeleteHelperInternal<>::DeleteViaSequencedTaskRunner<>()
#15 0x7fa0452c2965 base::SequencedTaskRunner::DeleteSoon<>()
#16 0x7fa0452c1edb policy::AsyncPolicyProvider::Shutdown()
#17 0x7fa0453a6a38 policy::BrowserPolicyConnector::Shutdown()
#18 0x0000047e4867 policy::BrowserPolicyConnectorChromeOS::Shutdown()
#19 0x000003257cef TestingBrowserProcess::SetBrowserPolicyConnector()
#20 0x000003258788 TestingBrowserProcess::SetLocalState()
#21 0x0000032569b0 ScopedTestingLocalState::~ScopedTestingLocalState()
#22 0x00000325ff27 TestingProfileManager::~TestingProfileManager()
#23 0x000001ee6789 chromeos::ProfileListChromeOSTest::~ProfileListChromeOSTest()
#24 0x000001ee6425
chromeos::ProfileListChromeOSTest_ActiveItem_Test::~ProfileListChromeOSTest_ActiveItem_Test()

There's already a similar early-out in
BrowserThreadImpl::FlushThreadPoolHelperForTesting(). Note that
PostTaskHelper() was already a no-op if it was called without an active globals
pool.

https://codereview.chromium.org/1086733002/diff/500001/chrome/browser/backgro...
File chrome/browser/background/background_mode_manager_unittest.cc (right):

https://codereview.chromium.org/1086733002/diff/500001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager_unittest.cc:170: // to avoid
having the browser's shutdown logic try to access a non-existent
On 2015/04/22 14:48:03, Andrew T Wilson wrote:
> I guess I'm not sure I understand - are you saying that there are shutdown
tasks
> that this dummy_task_runner_ won't execute? I'm not 100% clear why this class
> gets a dummy_task_runner_ but BackgroundModeManagerWithExtensionTest does not?

Disclaimer: that was just the first thing I managed to make work -- I don't
actually know anything about these tests.

I've now done some more digging and the underlying problem is that
chrome::DecrementKeepAliveCount() uses the absence of a MessageLoop to infer
that it's in a unit test and shouldn't do a real browser shutdown. Since my
patch is adding a MessageLoop, things go wrong when the shutdown sequence is
trying to touch things that aren't there.

Unfortunately I can't seem to find the set of objects that actually need to be
around for the shutdown to work (MessageCenter is enough for everything except
CrOS which wants an ash::Shell). BackgroundModeManagerWithExtensionTest works
around this by artificially incrementing the browser keepalive count to prevent
the shutdown, but we can't really do that in BackgroundModeManagerTest because
it is explicitly checking what the current keepalive count is.

The best solution I could think of is to add a
chrome::DisableShutdownForTesting() that this test can use to keep its existing
behavior. WDYT?

[jam, 2015-04-23 17:09:37]

On 2015/04/22 18:06:47, Sami wrote:
> Thanks for having a look.
> 
> On 2015/04/22 15:41:18, jam wrote:
> > my only comment is i don't understand why the browserthread change is
needed?
> 
> It was motivated by this DCHECK failure during test teardown:
> 
> [21975:21975:0422/182840:2544456752510:FATAL:thread_task_runner_handle.cc(23)]
> Check failed: current. 
> #0 0x7fa04653bc8e base::debug::StackTrace::StackTrace()
> #1 0x7fa04658e22f logging::LogMessage::~LogMessage()
> #2 0x7fa04666c067 base::ThreadTaskRunnerHandle::Get()
> #3 0x7fa046671d9a base::SequencedWorkerPool::SequencedWorkerPool()
> #4 0x7fa04f3de89b content::(anonymous
> namespace)::BrowserThreadGlobals::BrowserThreadGlobals()
> #5 0x7fa04f3de81a base::DefaultLazyInstanceTraits<>::New()
> #6 0x7fa04f3de745 base::internal::LeakyLazyInstanceTraits<>::New()
> #7 0x7fa04f3de6fd base::LazyInstance<>::Pointer()
> #8 0x7fa04f3dcbd5 base::LazyInstance<>::Get()
> #9 0x7fa04f3dd896 content::BrowserThreadImpl::PostTaskHelper()
> #10 0x7fa04f3de276 content::BrowserThread::PostNonNestableDelayedTask()
> #11 0x7fa04f3deca8 content::(anonymous
> namespace)::BrowserThreadMessageLoopProxy::PostNonNestableDelayedTask()
> #12 0x7fa046611ab9 base::SequencedTaskRunner::PostNonNestableTask()
> #13 0x7fa046611b23 base::SequencedTaskRunner::DeleteSoonInternal()
> #14 0x7fa0452c3a27
> base::subtle::DeleteHelperInternal<>::DeleteViaSequencedTaskRunner<>()
> #15 0x7fa0452c2965 base::SequencedTaskRunner::DeleteSoon<>()
> #16 0x7fa0452c1edb policy::AsyncPolicyProvider::Shutdown()
> #17 0x7fa0453a6a38 policy::BrowserPolicyConnector::Shutdown()
> #18 0x0000047e4867 policy::BrowserPolicyConnectorChromeOS::Shutdown()
> #19 0x000003257cef TestingBrowserProcess::SetBrowserPolicyConnector()
> #20 0x000003258788 TestingBrowserProcess::SetLocalState()
> #21 0x0000032569b0 ScopedTestingLocalState::~ScopedTestingLocalState()
> #22 0x00000325ff27 TestingProfileManager::~TestingProfileManager()
> #23 0x000001ee6789
chromeos::ProfileListChromeOSTest::~ProfileListChromeOSTest()
> #24 0x000001ee6425
>
chromeos::ProfileListChromeOSTest_ActiveItem_Test::~ProfileListChromeOSTest_ActiveItem_Test()

Can we fix the above by instantiating a browser thread bundle that outlives the
object being destructued? or does one already exist? 

> 
> There's already a similar early-out in
> BrowserThreadImpl::FlushThreadPoolHelperForTesting(). Note that
> PostTaskHelper() was already a no-op if it was called without an active
globals
> pool.

the difference is the other method is a testing only one. so i don't care as
much about null checks in there. i worry about doing this in production code
will allow other tasks to silently be dropped (since we generally don't monitor
PostTask return code for other reasons)

[Sami, 2015-04-23 18:14:51]

On 2015/04/23 17:09:37, jam wrote:
> Can we fix the above by instantiating a browser thread bundle that outlives
the
> object being destructued? or does one already exist? 

Ah, you're correct. I somehow misread the code as requiring something more than
a browser thread bundle, but looks like that is indeed sufficient.
 
> > 
> > There's already a similar early-out in
> > BrowserThreadImpl::FlushThreadPoolHelperForTesting(). Note that
> > PostTaskHelper() was already a no-op if it was called without an active
> globals
> > pool.
> 
> the difference is the other method is a testing only one. so i don't care as
> much about null checks in there. i worry about doing this in production code
> will allow other tasks to silently be dropped (since we generally don't
monitor
> PostTask return code for other reasons)

Fair point. It looks like we can avoid touching production code in this case --
which I agree is preferable.

[Sami, 2015-04-23 18:15:22]

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

[commit-bot: I haz the power, 2015-04-23 18:16:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1086733002/540001

[commit-bot: I haz the power, 2015-04-23 18:57:17]

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

[commit-bot: I haz the power, 2015-04-23 18:57:18]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jam, 2015-04-23 21:37:04]

lgtm, thanks for bearing with the slowness :)

[Andrew T Wilson (Slow), 2015-04-27 09:48:27]

background/ LGTM, but see my note about your change to application_lifetime.cc -
I'm just concerned that people that use your new API may end up skipping
shutdown if they call it after the process should be shutdown.

https://codereview.chromium.org/1086733002/diff/560001/chrome/browser/lifetim...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/1086733002/diff/560001/chrome/browser/lifetim...
chrome/browser/lifetime/application_lifetime.cc:427:
g_disable_shutdown_for_testing = disable_shutdown_for_testing;
Should setting this to false also initiate a shutdown using the same criteria as
DecrementKeepAliveCount()?

[Sami, 2015-04-27 11:31:41]

Thanks all!

https://codereview.chromium.org/1086733002/diff/560001/chrome/browser/lifetim...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/1086733002/diff/560001/chrome/browser/lifetim...
chrome/browser/lifetime/application_lifetime.cc:427:
g_disable_shutdown_for_testing = disable_shutdown_for_testing;
On 2015/04/27 09:48:26, Andrew T Wilson wrote:
> Should setting this to false also initiate a shutdown using the same criteria
as
> DecrementKeepAliveCount()?

That's a good thought, but unfortunately in BackgroundModeManagerTest it leads
back to the original problem since at the time of this call we don't have the
necessary context to do a proper shutdown (e.g., no MessageCenter instance).

I've now changed this to perform the shutdown like you suggested, but also made
BackgroundModeManagerTest ensure that we don't try to close the browser when we
shouldn't.

[Andrew T Wilson (Slow), 2015-04-27 15:04:48]

lgtm

[Sami, 2015-04-27 15:57:49]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-04-27 15:57:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1086733002/#ps640001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-04-27 15:58:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1086733002/640001

[commit-bot: I haz the power, 2015-04-27 17:59:48]

Committed patchset #35 (id:640001)

[commit-bot: I haz the power, 2015-04-27 18:00:39]

Patchset 35 (id:??) landed as
https://crrev.com/0becb33779f8678e2703d6ff1a73ce2b45571f98
Cr-Commit-Position: refs/heads/master@{#327067}