Create an AtExitManager around each test, to take care of stray singletons

Create an AtExitManager around each test, to take care of stray singletons

BUG=133403
TEST=unit tests run without AtExitManager warnings, and can run gtest_shuffled

[Scott Byer, 2012-07-11 22:21:41]

Something like this?

[Ryan Sleevi, 2012-07-11 22:30:41]

Yeah, this is exactly what I was thinking.

However, from looking at the gtest sources, it looks like it may also be
necessary to hook OnTestCaseStart/OnTestCaseEnd, since it's possible that the
test case may have a SetUpTestCase/TearDownTestCase methods, which run before
individual tests.

http://codereview.chromium.org/10735063/diff/1/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/10735063/diff/1/base/test/test_suite.cc#newcode93
base/test/test_suite.cc:93: " most likely because one was created without being
destroyed.";
style nit: The << should be on the beginning of each of these new lines (
http://www.chromium.org/developers/coding-style#TOC-Code-formatting "An
exception is log messages"

Same on line 105

[Scott Byer, 2012-07-11 22:35:18]

I'm thinking that should be two AtExitManagers then, one around individual tests
and an additional outer one around the test case.

The minimalist in me is tempted to just to the test case only one, but I don't
think that would really solve the gtest_shuffle case completely.

Thoughts?

On 2012/07/11 22:30:41, Ryan Sleevi wrote:
> Yeah, this is exactly what I was thinking.
> 
> However, from looking at the gtest sources, it looks like it may also be
> necessary to hook OnTestCaseStart/OnTestCaseEnd, since it's possible that the
> test case may have a SetUpTestCase/TearDownTestCase methods, which run before
> individual tests.
> 
> http://codereview.chromium.org/10735063/diff/1/base/test/test_suite.cc
> File base/test/test_suite.cc (right):
> 
>
http://codereview.chromium.org/10735063/diff/1/base/test/test_suite.cc#newcode93
> base/test/test_suite.cc:93: " most likely because one was created without
being
> destroyed.";
> style nit: The << should be on the beginning of each of these new lines (
> http://www.chromium.org/developers/coding-style#TOC-Code-formatting "An
> exception is log messages"
> 
> Same on line 105

[Ryan Sleevi, 2012-07-11 22:40:16]

On 2012/07/11 22:35:18, Scott Byer wrote:
> I'm thinking that should be two AtExitManagers then, one around individual
tests
> and an additional outer one around the test case.

Yes, that sounds correct to me.

> 
> The minimalist in me is tempted to just to the test case only one, but I don't
> think that would really solve the gtest_shuffle case completely.
> 
> Thoughts?

Right, it won't solve the shuffle case for any test which uses globals in
SetUpTestCase.

I don't know if there are any, but I also don't know that there aren't any.

Note that it's still possible for there to be 'other' globals that affect
gtest_shuffle (eg: there was a while where media_unittests was getting tripped
up by an ffmpeg global that could not be safely/correctly restored), but I think
it does provide a more hermetic environment for Chromium.

Although maruel@ is working on having all tests run in wholly isolated processes
for every test iteration, at which point there will be an even greater chance
that people 'accidentally' break this code (since it'll pass on trybots, but not
locally)

[Robert Sesek, 2012-07-13 20:50:17]

I've tried this before. You'll hit a wall in test_shell_tests.

[Robert Sesek, 2012-07-13 20:52:18]

Hit send too early!

This was my CL: http://codereview.chromium.org/8947021/

But I've heard tony@ is trying to kill test_shell_tests, so this may be able to
move forward in the near future. 

Also note that anything that uses StaticMemorySingletonTraits must be handled
specially.

[Scott Byer, 2012-07-13 21:59:49]

So, the thing that puzzles me is that these failures are on the try bot, yet
don't happen locally. test_shell_tests run find on mac and linux.

I wouldn't mind lining up the next yak if I could debug out a pointer to it.

Did you figure out anything from your attempt at it?

On 2012/07/13 20:52:18, rsesek wrote:
> Hit send too early!
> 
> This was my CL: http://codereview.chromium.org/8947021/
> 
> But I've heard tony@ is trying to kill test_shell_tests, so this may be able
to
> move forward in the near future. 
> 
> Also note that anything that uses StaticMemorySingletonTraits must be handled
> specially.

[Robert Sesek, 2012-07-13 22:10:27]

On 2012/07/13 21:59:49, Scott Byer wrote:
> So, the thing that puzzles me is that these failures are on the try bot, yet
> don't happen locally. test_shell_tests run find on mac and linux.
> 
> I wouldn't mind lining up the next yak if I could debug out a pointer to it.
> 
> Did you figure out anything from your attempt at it?

It seemed unsolvable easily. The test_shell_tests basically starts up the IO
thread (and background workers) on-demand but never shuts them down at the end
of the test. So while the MessageLoop stops in between tests, it may still have
tasks on it from the previous test that then get pumped in the next test and are
using-after-free Singleton objects.

FWIW I saw the test_shell failures locally.

Also, http://code.google.com/p/chromium/issues/detail?id=113008 may be a problem
on Windows.

[Ryan Sleevi, 2013-06-14 17:49:27]

On 2012/07/13 22:10:27, rsesek wrote:
> On 2012/07/13 21:59:49, Scott Byer wrote:
> > So, the thing that puzzles me is that these failures are on the try bot, yet
> > don't happen locally. test_shell_tests run find on mac and linux.
> > 
> > I wouldn't mind lining up the next yak if I could debug out a pointer to it.
> > 
> > Did you figure out anything from your attempt at it?
> 
> It seemed unsolvable easily. The test_shell_tests basically starts up the IO
> thread (and background workers) on-demand but never shuts them down at the end
> of the test. So while the MessageLoop stops in between tests, it may still
have
> tasks on it from the previous test that then get pumped in the next test and
are
> using-after-free Singleton objects.
> 
> FWIW I saw the test_shell failures locally.
> 
> Also, http://code.google.com/p/chromium/issues/detail?id=113008 may be a
problem
> on Windows.

Ping on this? Good to close?

[Scott Byer, 2013-06-14 17:52:00]

On 2013/06/14 17:49:27, Ryan Sleevi wrote:
> On 2012/07/13 22:10:27, rsesek wrote:
> > On 2012/07/13 21:59:49, Scott Byer wrote:
> > > So, the thing that puzzles me is that these failures are on the try bot,
yet
> > > don't happen locally. test_shell_tests run find on mac and linux.
> > > 
> > > I wouldn't mind lining up the next yak if I could debug out a pointer to
it.
> > > 
> > > Did you figure out anything from your attempt at it?
> > 
> > It seemed unsolvable easily. The test_shell_tests basically starts up the IO
> > thread (and background workers) on-demand but never shuts them down at the
end
> > of the test. So while the MessageLoop stops in between tests, it may still
> have
> > tasks on it from the previous test that then get pumped in the next test and
> are
> > using-after-free Singleton objects.
> > 
> > FWIW I saw the test_shell failures locally.
> > 
> > Also, http://code.google.com/p/chromium/issues/detail?id=113008 may be a
> problem
> > on Windows.
> 
> Ping on this? Good to close?

Yup, closed.