Remove direct usage of BrowserThreadImpl in tests

Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.

BUG=653916
Committed: https://crrev.com/606d46cc6d881eb8553c1d7755458913e0e1af73
Cr-Commit-Position: refs/heads/master@{#431070}

[gab, 2016-11-09 16:02:24]

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

[gab, 2016-11-09 16:02:32]

Description was changed from

==========
Remove direct usage of BrowserThreadImpl in tests


Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.
BUG=653916
==========

to

==========
Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.
BUG=653916
==========

[gab, 2016-11-09 16:02:32]

Description was changed from

==========
Remove direct usage of BrowserThreadImpl in tests


Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.
BUG=653916
==========

to

==========
Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.
BUG=653916
==========

[commit-bot: I haz the power, 2016-11-09 16:02:44]

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-09 16:05:12]

gab@chromium.org changed reviewers:
+ avi@chromium.org, pauljensen@chromium.org

[gab, 2016-11-09 16:05:13]

Avi PTAL, this is a precursor to https://codereview.chromium.org/2464233002/.

TBR pauljensen@ for documentation side-effects in
net/url_request/test_url_fetcher_factory.h

Thanks!
Gab

[pauljensen, 2016-11-09 16:25:00]

net/ lgtm

[Avi (use Gerrit), 2016-11-09 17:12:41]

Yes, LGTM. Can we do anything to prevent similar code from being written in the
future?

[commit-bot: I haz the power, 2016-11-09 17:43:22]

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

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

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

[gab, 2016-11-09 19:02:33]

On 2016/11/09 17:12:41, Avi wrote:
> Yes, LGTM. Can we do anything to prevent similar code from being written in
the
> future?

Yes, after https://codereview.chromium.org/2464233002/, test code won't work if
BrowserThread::ResetGlobalsForTesting() isn't called and it's a protected method
so it's only invokable by TestBrowserThread (and I sure hope that someone
overriding BrowserThreadImpl to call its protected methods in a test would be
flagged as bad -- especially with the comment I added in its header in this
CL!).

[gab, 2016-11-09 19:03:56]

On 2016/11/09 19:02:33, gab wrote:
> On 2016/11/09 17:12:41, Avi wrote:
> > Yes, LGTM. Can we do anything to prevent similar code from being written in
> the
> > future?
> 
> Yes, after https://codereview.chromium.org/2464233002/, test code won't work
if
> BrowserThread::ResetGlobalsForTesting() isn't called and it's a protected
method
> so it's only invokable by TestBrowserThread (and I sure hope that someone
> overriding BrowserThreadImpl to call its protected methods in a test would be
> flagged as bad -- especially with the comment I added in its header in this
> CL!).

PS: The reason it won't work if this isn't clear is that globals.states ends up
in SHUTDOWN phase and each phase change checks the previous phase (i.e.
INITIALIZED expects to come after UNINITIALIZED) so if it INITIALIZED comes
after SHUTDOWN a DCHECK fires).

[gab, 2016-11-09 19:04:13]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-11-09 19:04:41]

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-09 20:47:42]

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

[commit-bot: I haz the power, 2016-11-09 20:47:43]

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

[gab, 2016-11-09 21:11:34]

Patchset #2 (id:20001) has been deleted

[gab, 2016-11-09 21:13:26]

gab@chromium.org changed reviewers:
+ mmenke@chromium.org

[gab, 2016-11-09 21:13:27]

+mmenke, fixing an ASAN error in content/browser/loader in PS1 caused by a
missing ResourceDispatcherHostImpl::Shutdown() call (to match BrowserMainLoop
and other unittests doing the same thing).

Not sure why that passed before this change though?

[gab, 2016-11-09 21:17:06]

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

[gab, 2016-11-09 21:17:45]

Description was changed from

==========
Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.
BUG=653916
==========

to

==========
Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.

BUG=653916
==========

[commit-bot: I haz the power, 2016-11-09 21:17:50]

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

[mmenke, 2016-11-09 21:36:13]

On 2016/11/09 21:13:27, gab wrote:
> +mmenke, fixing an ASAN error in content/browser/loader in PS1 caused by a
> missing ResourceDispatcherHostImpl::Shutdown() call (to match BrowserMainLoop
> and other unittests doing the same thing).
> 
> Not sure why that passed before this change though?

LGTM!  Thanks for adding me for that fix.  And for those watching at home, turns
out the test doesn't need an RDH, anyways, so we just removed it.

[gab, 2016-11-09 21:36:48]

The CQ bit was checked by gab@chromium.org

[gab, 2016-11-09 21:36:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2487073005/#ps60001
(title: "The RDHImpl is actually not even necessary in ResourceSchedulerTest,
removed.")

[commit-bot: I haz the power, 2016-11-09 21:37:15]

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-09 23:07:48]

Description was changed from

==========
Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.

BUG=653916
==========

to

==========
Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.

BUG=653916
==========

[commit-bot: I haz the power, 2016-11-09 23:07:50]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-09 23:15:55]

Description was changed from

==========
Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.

BUG=653916
==========

to

==========
Remove direct usage of BrowserThreadImpl in tests

Instantiating a BrowserThreadImpl sets global state which leaks to the next
tests. This is a problem in https://codereview.chromium.org/2464233002/ where
a reset was introduced for tests (driven by TestBrowserThread).

Tests should already have been using TestBrowserThread over BrowserThreadImpl
and today TestBrowserThreadBundle is favored over TestBrowserThread so it is
used as the replacement in this CL.

BUG=653916

Committed: https://crrev.com/606d46cc6d881eb8553c1d7755458913e0e1af73
Cr-Commit-Position: refs/heads/master@{#431070}
==========

[commit-bot: I haz the power, 2016-11-09 23:15:56]

Patchset 3 (id:??) landed as
https://crrev.com/606d46cc6d881eb8553c1d7755458913e0e1af73
Cr-Commit-Position: refs/heads/master@{#431070}