Use weak_ptr in bind call to TraceLog::OnFlushTimeout

Use weak_ptr in bind call to TraceLog::OnFlushTimeout

The reason for this change is that OnFlushTimeout callback
can run after TraceLog is destroyed for testing, if the message
loop that the task gets posted to is not destroyed.

(This undesirable effect can currently be seen in
https://codereview.chromium.org/468083004/, which causes
one of ios_dbg_simulator's net_unittests to crash.)

BUG=

[xunjieli, 2014-09-12 17:16:08]

xunjieli@chromium.org changed reviewers:
+ dsinclair@chromium.org, mmenke@chromium.org, nduca@chromium.org,
wangxianzhu@chromium.org

[xunjieli, 2014-09-12 17:16:08]

Hi wangxianzhu, nduca, dsinclair, and mmenke, 

Please take a look at my CL. Thank you!

Helen

[mmenke, 2014-09-12 17:47:46]

I defer to others on the best way to fix this issue - not sure what the expected
use case of flushing trace events is.

https://codereview.chromium.org/568073003/diff/1/base/debug/trace_event_impl.cc
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/568073003/diff/1/base/debug/trace_event_impl....
base/debug/trace_event_impl.cc:1731: flush_message_loop_proxy_->PostDelayedTask(
The weak pointer factory can only vend weak points on the thread the TraceLog
lives on, and its weak pointers can only be dereferenced on the same thread.

As a result, this only works if the TraceLog is created and destroyed on the
flush_message_loop_proxy_'s message loop.

The real issue is that the message loop outlives the TraceLog, which breaks the
underlying assumptions of this code.

[Xianzhu, 2014-09-15 16:35:50]

Unretained(this) should safe because TraceLog is a leaky global object which
will be never deleted.

TraceLog::DeleteForTesting() is an exception. It's safe in base_unittests
because there is no message loop in the main thread.

If a test having message loop in the main thread needs to delete TraceLog, it
should ensure the message loop is stopped before deleting TraceLog, or it should
not use TraceLog::DeleteForTesting.

[xunjieli, 2014-09-15 20:53:05]

On 2014/09/15 16:35:50, Xianzhu wrote:
> Unretained(this) should safe because TraceLog is a leaky global object which
> will be never deleted.
> 
> TraceLog::DeleteForTesting() is an exception. It's safe in base_unittests
> because there is no message loop in the main thread.
> 
> If a test having message loop in the main thread needs to delete TraceLog, it
> should ensure the message loop is stopped before deleting TraceLog, or it
should
> not use TraceLog::DeleteForTesting.

Thanks Xianzhu. Matt, do you know if there's a way to stop the message loop in
net_unittest from running? (my unit test is at
https://codereview.chromium.org/468083004/patch/250001/260005). Thanks!

[mmenke, 2014-09-15 20:55:02]

On 2014/09/15 20:53:05, xunjieli1 wrote:
> On 2014/09/15 16:35:50, Xianzhu wrote:
> > Unretained(this) should safe because TraceLog is a leaky global object which
> > will be never deleted.
> > 
> > TraceLog::DeleteForTesting() is an exception. It's safe in base_unittests
> > because there is no message loop in the main thread.
> > 
> > If a test having message loop in the main thread needs to delete TraceLog,
it
> > should ensure the message loop is stopped before deleting TraceLog, or it
> should
> > not use TraceLog::DeleteForTesting.
> 
> Thanks Xianzhu. Matt, do you know if there's a way to stop the message loop in
> net_unittest from running? (my unit test is at
> https://codereview.chromium.org/468083004/patch/250001/260005). Thanks!

There's not - the net/ test fixture sets up the message loop, and it's shared
between all tests.

[mmenke, 2014-09-15 21:01:31]

On 2014/09/15 20:55:02, mmenke wrote:
> On 2014/09/15 20:53:05, xunjieli1 wrote:
> > On 2014/09/15 16:35:50, Xianzhu wrote:
> > > Unretained(this) should safe because TraceLog is a leaky global object
which
> > > will be never deleted.
> > > 
> > > TraceLog::DeleteForTesting() is an exception. It's safe in base_unittests
> > > because there is no message loop in the main thread.
> > > 
> > > If a test having message loop in the main thread needs to delete TraceLog,
> it
> > > should ensure the message loop is stopped before deleting TraceLog, or it
> > should
> > > not use TraceLog::DeleteForTesting.
> > 
> > Thanks Xianzhu. Matt, do you know if there's a way to stop the message loop
in
> > net_unittest from running? (my unit test is at
> > https://codereview.chromium.org/468083004/patch/250001/260005). Thanks!
> 
> There's not - the net/ test fixture sets up the message loop, and it's shared
> between all tests.

So that leaves us with 4 options:

1)  Don't delete the TraceLog in the unit test.
2)  Refactor the test fixture used by all net unit tests to create a MessageLoop
for every test.  This seems like a non-starter.  Even if we did it, if the
TraceLog is destroyed before the MessageLoop, we have problems, so we can't
create a MessageLoop in the test fixture at all.
3)  Don't run on the net's test fixture's MessageLoop.  Unfortunately, we can't
just create a new one to mask the old one.  We'd have to create a new thread,
and I'm not entirely sure that would actually work - depends on if
TraceLog::Flush posts a task to the net test fixture's MessageLoop or not.
4)  Fix TraceLog::Flush not to depend on being alive as long as the message loop
its on.

If we can 1) working, it's probably the simplest option.

[xunjieli, 2014-09-15 21:13:04]

On 2014/09/15 21:01:31, mmenke wrote:
> On 2014/09/15 20:55:02, mmenke wrote:
> > On 2014/09/15 20:53:05, xunjieli1 wrote:
> > > On 2014/09/15 16:35:50, Xianzhu wrote:
> > > > Unretained(this) should safe because TraceLog is a leaky global object
> which
> > > > will be never deleted.
> > > > 
> > > > TraceLog::DeleteForTesting() is an exception. It's safe in
base_unittests
> > > > because there is no message loop in the main thread.
> > > > 
> > > > If a test having message loop in the main thread needs to delete
TraceLog,
> > it
> > > > should ensure the message loop is stopped before deleting TraceLog, or
it
> > > should
> > > > not use TraceLog::DeleteForTesting.
> > > 
> > > Thanks Xianzhu. Matt, do you know if there's a way to stop the message
loop
> in
> > > net_unittest from running? (my unit test is at
> > > https://codereview.chromium.org/468083004/patch/250001/260005). Thanks!
> > 
> > There's not - the net/ test fixture sets up the message loop, and it's
shared
> > between all tests.
> 
> So that leaves us with 4 options:
> 
> 1)  Don't delete the TraceLog in the unit test.
> 2)  Refactor the test fixture used by all net unit tests to create a
MessageLoop
> for every test.  This seems like a non-starter.  Even if we did it, if the
> TraceLog is destroyed before the MessageLoop, we have problems, so we can't
> create a MessageLoop in the test fixture at all.
> 3)  Don't run on the net's test fixture's MessageLoop.  Unfortunately, we
can't
> just create a new one to mask the old one.  We'd have to create a new thread,
> and I'm not entirely sure that would actually work - depends on if
> TraceLog::Flush posts a task to the net test fixture's MessageLoop or not.
> 4)  Fix TraceLog::Flush not to depend on being alive as long as the message
loop
> its on.
> 
> If we can 1) working, it's probably the simplest option.

1) seems to work on my local machine. Thanks for the suggestions! I will fix my
other CL.