Destroy all Singletons and LazyInstances between each test.

Destroy all Singletons and LazyInstances between each test.

This modifies base::TestSuite to add a test listener that runs the AtExitManager
between each test.

BUG=110594, 156433, 238654
TEST=*unit*tests
R=apatrick@chromium.org, phajdan.jr@chromium.org, pneubeck@chromium.org, rsleevi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202305
Reverted: https://src.chromium.org/viewvc/chrome?view=rev&revision=202312

[Robert Sesek, 2013-05-07 18:06:20]

I think this may stick this time!

apactrick: gpu/
phajdan: base/test/

[jam, 2013-05-07 18:09:56]

https://codereview.chromium.org/8947021/diff/68002/base/test/test_suite.h
File base/test/test_suite.h (right):

https://codereview.chromium.org/8947021/diff/68002/base/test/test_suite.h#new...
base/test/test_suite.h:65: virtual void EnableSingletonTestIsolation();
i highly suggest not adding this method, i.e. not having a way for tests to get
out of this correct mode.

when we add things like ScopedAllowIO, they  just get misused

[Robert Sesek, 2013-05-07 18:12:29]

https://codereview.chromium.org/8947021/diff/68002/base/test/test_suite.h
File base/test/test_suite.h (right):

https://codereview.chromium.org/8947021/diff/68002/base/test/test_suite.h#new...
base/test/test_suite.h:65: virtual void EnableSingletonTestIsolation();
On 2013/05/07 18:09:57, jam wrote:
> i highly suggest not adding this method, i.e. not having a way for tests to
get
> out of this correct mode.
> 
> when we add things like ScopedAllowIO, they  just get misused

Good point. Done.

[apatrick_chromium, 2013-05-07 19:16:41]

lgtm

[Robert Sesek, 2013-05-07 23:58:55]

Pawel: ping

[Paweł Hajdan Jr., 2013-05-08 16:32:25]

https://codereview.chromium.org/8947021/diff/73002/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/8947021/diff/73002/base/test/test_suite.cc#ne...
base/test/test_suite.cc:80: // between tests.
nit: Let's add some text here that it helps prevent carrying state from one test
to another, which is a frequent cause of mysterious and hard to debug issues.

https://codereview.chromium.org/8947021/diff/73002/base/test/test_suite.cc#ne...
base/test/test_suite.cc:279: // test.
nit: Also expand this comment. To avoid duplication, feel free to point to the
SingletonDestructor comment or the bug...

https://codereview.chromium.org/8947021/diff/73002/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer_unittest.cc (right):

https://codereview.chromium.org/8947021/diff/73002/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer_unittest.cc:158: // Doesn't work
with singleton isolation <http://crbug.com/238654>.
Please find a good owner for this bug, and let the owner sign off on this patch
*before* it is checked in.

I think you're hitting the same problems I hit before - and maybe that's indeed
the way to handle it for now (it's a huge improvement for stability of the
tests), but hey, we're really decreasing our test coverage in a pretty important
area.

https://codereview.chromium.org/8947021/diff/73002/gpu/command_buffer/service...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/8947021/diff/73002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_translator.cc:17: static bool
g_shader_initalized_ = false;
nit: No underscore at the end for global variables.

https://codereview.chromium.org/8947021/diff/73002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_translator.cc:21: g_shader_initalized_ =
false;
Let's DCHECK before it that g_shader_initialized is true.

[Robert Sesek, 2013-05-08 16:42:11]

+pneubeck for cros

https://codereview.chromium.org/8947021/diff/73002/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/8947021/diff/73002/base/test/test_suite.cc#ne...
base/test/test_suite.cc:80: // between tests.
On 2013/05/08 16:32:25, Paweł Hajdan Jr. wrote:
> nit: Let's add some text here that it helps prevent carrying state from one
test
> to another, which is a frequent cause of mysterious and hard to debug issues.

Done.

https://codereview.chromium.org/8947021/diff/73002/base/test/test_suite.cc#ne...
base/test/test_suite.cc:279: // test.
On 2013/05/08 16:32:25, Paweł Hajdan Jr. wrote:
> nit: Also expand this comment. To avoid duplication, feel free to point to the
> SingletonDestructor comment or the bug...

Done.

https://codereview.chromium.org/8947021/diff/73002/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer_unittest.cc (right):

https://codereview.chromium.org/8947021/diff/73002/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer_unittest.cc:158: // Doesn't work
with singleton isolation <http://crbug.com/238654>.
On 2013/05/08 16:32:25, Paweł Hajdan Jr. wrote:
> Please find a good owner for this bug, and let the owner sign off on this
patch
> *before* it is checked in.
> 
> I think you're hitting the same problems I hit before - and maybe that's
indeed
> the way to handle it for now (it's a huge improvement for stability of the
> tests), but hey, we're really decreasing our test coverage in a pretty
important
> area.

Done. I think the net win outweighs the importance of any one test, though.

https://codereview.chromium.org/8947021/diff/73002/gpu/command_buffer/service...
File gpu/command_buffer/service/shader_translator.cc (right):

https://codereview.chromium.org/8947021/diff/73002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_translator.cc:17: static bool
g_shader_initalized_ = false;
On 2013/05/08 16:32:25, Paweł Hajdan Jr. wrote:
> nit: No underscore at the end for global variables.

Done.

https://codereview.chromium.org/8947021/diff/73002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_translator.cc:21: g_shader_initalized_ =
false;
On 2013/05/08 16:32:25, Paweł Hajdan Jr. wrote:
> Let's DCHECK before it that g_shader_initialized is true.

Done.

[Paweł Hajdan Jr., 2013-05-08 23:33:53]

LGTM

[Robert Sesek, 2013-05-09 14:29:44]

+gspencer for chromeos/network stamp

[Greg Spencer (Chromium), 2013-05-09 17:23:29]

I understand why this is important, and that it is more correct, and I agree we
should investigate fixing this. But note that because of the way NSS operates,
it may not be possible to make singleton isolation work for these tests without
making and upstreaming significant changes to NSS, which could take a long time.

In the meantime, you are basically turning off certificate import testing
completely, so can you give us some time to investigate the bug you filed before
you turn this on?

I know you're probably excited to get this change in, given how long it's been
around, but I'd rather not go without testing for that long if we can help it.

Is there a way to check in your change without disabling these tests?

[jam, 2013-05-09 17:26:27]

On 2013/05/09 17:23:29, Greg Spencer (Chromium) wrote:
> I understand why this is important, and that it is more correct, and I agree
we
> should investigate fixing this. But note that because of the way NSS operates,
> it may not be possible to make singleton isolation work for these tests
without
> making and upstreaming significant changes to NSS, which could take a long
time.
> 
> In the meantime, you are basically turning off certificate import testing
> completely, so can you give us some time to investigate the bug you filed
before
> you turn this on?
> 
> I know you're probably excited to get this change in, given how long it's been
> around, but I'd rather not go without testing for that long if we can help it.
> 
> Is there a way to check in your change without disabling these tests?

note: an option is to turn this on, but move
onc_certificate_importer_unittest.cc to the browser_tests target. then every
test would run in its own process. we do this for all the RenderView tests for
example, which each need to initialize webkit and that can only happen once per
process. but we'd want to file a bug to track this issue

[Greg Spencer (Chromium), 2013-05-09 17:28:00]

On 2013/05/09 17:26:27, jam wrote:
> note: an option is to turn this on, but move
> onc_certificate_importer_unittest.cc to the browser_tests target. then every
> test would run in its own process.

This seems like a reasonable compromise to me.

[Ilya Sherman, 2013-05-09 23:13:09]

Incidentally, does this fix [
https://code.google.com/p/chromium/issues/detail?id=125931 ]?  If so, could you
update the comment in [
https://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/geolo...
] to no longer scare folks off from calling OverrideLocationForTesting() in
unit_tests?  Thanks :)

[Robert Sesek, 2013-05-22 14:52:34]

I've tracked down the problem with the ChromeOS test to be NSS-related. I got
most tests passing by actually closing the cert database in ~ScopedTestNSSDB().
Unfortunately, a few ONCCertificateImporterTests still failed due to a timing
(!) issue in NSS. Adding a 1-second sleep before the problematic call fixes it.
It's really a gross hack, but it makes this CL landable without disabling any
tests. Thoughts?

isherman: It's possible that this will fix it. I haven't tested it though, but
feel free to do so if/when it lands.

[Ryan Sleevi, 2013-05-22 19:50:59]

https://codereview.chromium.org/8947021/diff/93002/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/8947021/diff/93002/crypto/nss_util.cc#newcode359
crypto/nss_util.cc:359: bool OpenTestNSSDB() {
I'm not fully sure I understood - is this a failure on linux, linux_chromeos, or
on real ChromeOS bots?

I suspect that the failure of the test is a side-effect of your changes on
749/750, and wholly independent of the particular test being run. That is, I
expect a variety of NSS tests to possibly fail *if* it's the timing issue I
think it is.

The timing issue I think it is comes from the 1 second constant in
http://mxr.mozilla.org/nss/source/lib/dev/devslot.c#17 , which is used by
http://mxr.mozilla.org/nss/source/lib/dev/devslot.c#102 attempting to avoid
polling hardware devices quicker than 1 second.

Because of how NSS's SECMOD DBs (that is, the filesystem-backed sqlite DB we're
using here on line 364) work, we simulate as if we're a hardware slot, thus I
suspect you're hitting that timer and it's not polling to see if the 'card' is
still present before it tries to use it.

A workaround, of sorts, that avoids the sleep, would be to try the following
modifications:

https://codereview.chromium.org/8947021/diff/93002/crypto/nss_util.cc#newcode364
crypto/nss_util.cc:364: test_slot_ = OpenUserDB(g_test_nss_db_dir.Get().path(),
"Test DB");
if (test_slot_)
  PK11_UserEnableSlot(test_slot_);

https://codereview.chromium.org/8947021/diff/93002/crypto/nss_util.cc#newcode369
crypto/nss_util.cc:369: if (test_slot_) {
if (!test_slot_)
  return;

PK11_UserDisableSlot(test_slot_);
SECStatus status = SECMOD_CloseUserDB(test_slot_);
...


If this doesn't work, then the sleep(1) call really belongs here, since it will
take up to 1 second before the state is 'fully' undone (that is, before NSS will
flush it's cache and attempt to re-query the "hardware")

[Robert Sesek, 2013-05-23 16:55:16]

https://codereview.chromium.org/8947021/diff/93002/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/8947021/diff/93002/crypto/nss_util.cc#newcode359
crypto/nss_util.cc:359: bool OpenTestNSSDB() {
On 2013/05/22 19:50:59, Ryan Sleevi wrote:
> I'm not fully sure I understood - is this a failure on linux, linux_chromeos,
or
> on real ChromeOS bots?

This is a failure on Linux with GYP_DEFINES=chromeos=1.

> I suspect that the failure of the test is a side-effect of your changes on
> 749/750, and wholly independent of the particular test being run. That is, I
> expect a variety of NSS tests to possibly fail *if* it's the timing issue I
> think it is.
> 
> The timing issue I think it is comes from the 1 second constant in
> http://mxr.mozilla.org/nss/source/lib/dev/devslot.c#17 , which is used by
> http://mxr.mozilla.org/nss/source/lib/dev/devslot.c#102 attempting to avoid
> polling hardware devices quicker than 1 second.
> 
> Because of how NSS's SECMOD DBs (that is, the filesystem-backed sqlite DB
we're
> using here on line 364) work, we simulate as if we're a hardware slot, thus I
> suspect you're hitting that timer and it's not polling to see if the 'card' is
> still present before it tries to use it.
> 
> A workaround, of sorts, that avoids the sleep, would be to try the following
> modifications:

I tried the UserEnable workaround, but it still results in these ONC tests
failing :/

[Ryan Sleevi, 2013-05-24 01:39:41]

The Sleep(1) is fine by me, even though it's a giant hack.

I've traced the bug further and filed
https://bugzilla.mozilla.org/show_bug.cgi?id=875601 , which I'll fix for NSS
3.15.1 (it's too late for NSS 3.15)

I have a 4 line fix for NSS that causes the test to succeed without the sleep,
as described in the upstream bug.

[Robert Sesek, 2013-05-24 02:40:56]

On 2013/05/24 01:39:41, Ryan Sleevi wrote:
> The Sleep(1) is fine by me, even though it's a giant hack.
> 
> I've traced the bug further and filed
> https://bugzilla.mozilla.org/show_bug.cgi?id=875601 , which I'll fix for NSS
> 3.15.1 (it's too late for NSS 3.15)
> 
> I have a 4 line fix for NSS that causes the test to succeed without the sleep,
> as described in the upstream bug.

Alright. Let's just live with this hacky Sleep() call until NSS can be patched
appropriately.

gspencer/pneubeck: Is this change now okay by you?

[pneubeck (no reviews), 2013-05-24 20:02:38]

Totally. lgtm.
And thanks for that thorough investigation!

[Greg Spencer (Chromium), 2013-05-24 20:13:39]

On 2013/05/24 20:02:38, pneubeck wrote:
> Totally. lgtm.
> And thanks for that thorough investigation!

Yeah, thanks for that.  I'm OK with the sleep(1) for now, given the reason why. 
LGTM

[Robert Sesek, 2013-05-24 20:23:59]

Thanks.

Ryan: Can I haz a crypto/ LG?

I'll attempt to land this on over the weekend when things are quiet! *Fingers
crossed*

[Ryan Sleevi, 2013-05-24 20:42:16]

lgtm

[Robert Sesek, 2013-05-25 17:42:26]

Committed patchset #12 manually as r202305.