Force non-joinable SimpleThreads to be leaked.

Force non-joinable SimpleThreads to be leaked.

The downstream iOS bots caught a race in https://codereview.chromium.org/2204333003/.

The race is reproducible on all platforms by forcing a sleep in SimpleThread::ThreadMain()
just before invoking Run().

The issue is that the tests return from Start() after the |event_| is signaled but there's
no guarantee that Run() is called before the SimpleThread object goes out of scope and
is destroyed. Making a race to invoke a virtual method on a potentially deleted object.
The iOS10 kernel just so happens to exercise that race more reliably. And worse, the crash
often occurred in the next, unrelated, test...

Non-joinable base::Threads are forced to be leaked and non-joinable base::SimpleThreads will
have to follow suit.

BUG=634835, 629716

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

Description was changed from

==========
Force non-joinable SimpleThreads to be leaked.

BUG=634835, 629716
==========

to

==========
Force non-joinable SimpleThreads to be leaked.

The upstream iOS bots caught a race in
https://codereview.chromium.org/2204333003/.

The race is reproducible on all platforms by forcing a sleep in
SimpleThread::ThreadMain()
just before invoking Run().

The issue is that the tests return from Start() after the |event_| is signaled
but there's
no guarantee that Run() is called before the SimpleThread object goes out of
scope and
is destroyed. Making a race to invoke a virtual method on a potentially deleted
object.
The iOS10 kernel just so happens to exercise that race more reliably. And worse,
the crash
often occurred in the next, unrelated, test...

Non-joinable base::Threads are forced to be leaked and non-joinable
base::SimpleThreads will
have to follow suit.

BUG=634835, 629716
==========

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

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

[gab, 2016-08-05 16:13:55]

Lei, PTAL, this addresses a live issue on the upstream iOS bots
(http://crbug.com/634835).

Thanks

[sdefresne, 2016-08-05 16:48:33]

sdefresne@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2016-08-05 16:48:34]

nit: in description, the bot is downstream, not upstream

https://codereview.chromium.org/2218983003/diff/1/base/threading/simple_threa...
File base/threading/simple_thread_unittest.cc (right):

https://codereview.chromium.org/2218983003/diff/1/base/threading/simple_threa...
base/threading/simple_thread_unittest.cc:151: SleepRunner runner;
I think SleepRunner also needs to be leaked as DelegateSimpleThread::Run will
call SleepRunner::Run which is a virtual method.

[gab, 2016-08-05 17:23:19]

Description was changed from

==========
Force non-joinable SimpleThreads to be leaked.

The upstream iOS bots caught a race in
https://codereview.chromium.org/2204333003/.

The race is reproducible on all platforms by forcing a sleep in
SimpleThread::ThreadMain()
just before invoking Run().

The issue is that the tests return from Start() after the |event_| is signaled
but there's
no guarantee that Run() is called before the SimpleThread object goes out of
scope and
is destroyed. Making a race to invoke a virtual method on a potentially deleted
object.
The iOS10 kernel just so happens to exercise that race more reliably. And worse,
the crash
often occurred in the next, unrelated, test...

Non-joinable base::Threads are forced to be leaked and non-joinable
base::SimpleThreads will
have to follow suit.

BUG=634835, 629716
==========

to

==========
Force non-joinable SimpleThreads to be leaked.

The downstream iOS bots caught a race in
https://codereview.chromium.org/2204333003/.

The race is reproducible on all platforms by forcing a sleep in
SimpleThread::ThreadMain()
just before invoking Run().

The issue is that the tests return from Start() after the |event_| is signaled
but there's
no guarantee that Run() is called before the SimpleThread object goes out of
scope and
is destroyed. Making a race to invoke a virtual method on a potentially deleted
object.
The iOS10 kernel just so happens to exercise that race more reliably. And worse,
the crash
often occurred in the next, unrelated, test...

Non-joinable base::Threads are forced to be leaked and non-joinable
base::SimpleThreads will
have to follow suit.

BUG=634835, 629716
==========

[gab, 2016-08-05 17:28:33]

https://codereview.chromium.org/2218983003/diff/1/base/threading/simple_threa...
File base/threading/simple_thread_unittest.cc (right):

https://codereview.chromium.org/2218983003/diff/1/base/threading/simple_threa...
base/threading/simple_thread_unittest.cc:151: SleepRunner runner;
On 2016/08/05 16:48:33, sdefresne (travelling - slow) wrote:
> I think SleepRunner also needs to be leaked as DelegateSimpleThread::Run will
> call SleepRunner::Run which is a virtual method.

Good point, for DelegateSimpleThread, the Delegate also needs to be leaked this
makes me think that it's okay to delete non-joinable SimpleThreads when they
have a Delegate, so long as the Delegate is deleted.

This is in fact my proposal to eventually not force base::Thread to be leaked
under similar circumstances (but it currently doesn't have a Delegate):
https://bugs.chromium.org/p/chromium/issues/detail?id=629139#c14

I'll change the CL to only force leaking the Delegate.

[Reilly Grant (use Gerrit), 2016-08-05 22:24:19]

Take a look at base_unittests under TSan as well before landing this. It looks
like two more tests may be failing because of this change.

https://build.chromium.org/p/chromium.memory.full/builders/Linux%20TSan%20Tes...

[gab, 2016-08-08 21:19:49]

Re-opened https://codereview.chromium.org/2204333003/ and will continue there
(closing this one).

On 2016/08/05 22:24:19, Reilly Grant wrote:
> Take a look at base_unittests under TSan as well before landing this. It looks
> like two more tests may be failing because of this change.
> 
>
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20TSan%20Tes...

Will make sure to run TSAN try bots before landing, thanks!