Fix use-after-free in base::Timer::StopAndAbandon()

Fix use-after-free in base::Timer::StopAndAbandon()

A timer may be owned by a ref counted class. If |Timer::user_task_| is a method
in the ref counted class, it is possible for |Timer::user_task_| to hold the
only reference to the ref counted class. This CL changes
Timer::StopAndAbandon() so that Timer::Stop() is called after
Timer::AbandonScheduledTask(). If |Timer::user_task_| holds the only reference
to ref counted class, Timer::Stop() destroys the timer object.

Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.

BUG=678592
TEST=MessageLoopShutdownSelfOwningTimer

Review-Url: https://codereview.chromium.org/2624133004
Cr-Commit-Position: refs/heads/master@{#444132}
Committed: https://chromium.googlesource.com/chromium/src/+/4a286ed6b9406edfb1031ae1d48766f79652a6ca

[pkotwicz, 2017-01-12 01:57:10]

Description was changed from

==========
Fix use-after-free in base::Timer::StopAndAbandon()

A timer may be owned by a ref counted class. If the timer task is a method in
the ref counted class, it is possible for |Timer::user_task_| to hold the only
reference to the ref counted class. This CL changes Timer::StopAndAbandon() so
that Timer::Stop() is called after Timer::AbandonScheduledTask(). If
|Timer::user_task_| holds the only reference to ref counted class, Timer::Stop()
destroys the timer object.

Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.

BUG=678592
TEST=MessageLoopShutdownSelfOwningTimer
==========

to

==========
Fix use-after-free in base::Timer::StopAndAbandon()

A timer may be owned by a ref counted class. If the timer task is a method in
the ref counted class, it is possible for |Timer::user_task_| to hold the only
reference to the ref counted class. This CL changes Timer::StopAndAbandon() so
that Timer::Stop() is called after Timer::AbandonScheduledTask(). If
|Timer::user_task_| holds the only reference to ref counted class,
Timer::Stop() destroys the timer object.

Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.

BUG=678592
TEST=MessageLoopShutdownSelfOwningTimer
==========

[pkotwicz, 2017-01-12 01:57:17]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-01-12 01:58:52]

Description was changed from

==========
Fix use-after-free in base::Timer::StopAndAbandon()

A timer may be owned by a ref counted class. If the timer task is a method in
the ref counted class, it is possible for |Timer::user_task_| to hold the only
reference to the ref counted class. This CL changes Timer::StopAndAbandon() so
that Timer::Stop() is called after Timer::AbandonScheduledTask(). If
|Timer::user_task_| holds the only reference to ref counted class,
Timer::Stop() destroys the timer object.

Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.

BUG=678592
TEST=MessageLoopShutdownSelfOwningTimer
==========

to

==========
Fix use-after-free in base::Timer::StopAndAbandon()

A timer may be owned by a ref counted class. If |Timer::user_task_| is a method
in the ref counted class, it is possible for |Timer::user_task_| to hold the
only reference to the ref counted class. This CL changes
Timer::StopAndAbandon() so that Timer::Stop() is called after
Timer::AbandonScheduledTask(). If |Timer::user_task_| holds the only reference
to ref counted class, Timer::Stop() destroys the timer object.

Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.

BUG=678592
TEST=MessageLoopShutdownSelfOwningTimer
==========

[pkotwicz, 2017-01-12 01:58:52]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2017-01-12 02:12:56]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2017-01-12 02:13:00]

Patchset #1 (id:40001) has been deleted

[pkotwicz, 2017-01-12 02:18:28]

Dominick can you please take a look? This CL should fix use-after-free in
AddToHomescreenDataFetcherTestCommon.ManifestNoNameNoShortName

Dominick can you double check the CL for correctness before I send it out to
base/ OWNERS. Naming and comments are hard. To prevent too many conflicting
opinions on the "ideal name" of variables, I am delegating finding the ideal
names and comments to the base/ OWNERS

[dominickn, 2017-01-12 03:27:17]

Nice catch - that is a gnarly case you've found. No obvious problems to me, but
I'm not an expert here. :)

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer.h#newc...
base/timer/timer.h:165: void StopAndAbandon() {
Note for base/timer owners: is it worth changing this to be called
AbandonAndStop()?

[pkotwicz, 2017-01-12 05:19:32]

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

[pkotwicz, 2017-01-12 05:19:36]

gab@ for base/ OWNERS

[gab, 2017-01-12 19:42:54]

lg, thanks!

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer.h#newc...
base/timer/timer.h:165: void StopAndAbandon() {
On 2017/01/12 03:27:17, dominickn wrote:
> Note for base/timer owners: is it worth changing this to be called
> AbandonAndStop()?

No as I'm rewriting all of base::Timer in upcoming
https://codereview.chromium.org/2491613004/ which doesn't have this problem
(although I hadn't explicitly thought of that, it seems I luckily wrote it in a
way that doesn't have this issue I think) -- but thanks for the fix in the mean
time, the regression test will be most useful to make sure the new impl behaves
as expected.

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer.h#newc...
base/timer/timer.h:169: // No more member accesses here: *this could be deleted
at this point.
s/*this/|this|/

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer_unitte...
File base/timer/timer_unittest.cc (right):

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer_unitte...
base/timer/timer_unittest.cc:575: ~OneShotSelfOwningTimerTester() {}
= default;

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer_unitte...
base/timer/timer_unittest.cc:578: if (did_run_)
Instead of using |did_run_| simply
  ADD_FAILURE() << "Timer unexpectedly fired.";
here and drop the EXPECT_FALSE in the test's body.

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer_unitte...
base/timer/timer_unittest.cc:597: {
Instead of using a nested scope and racily depending on it expiring before the
timer fires, let's std::move(tester)->StartTimer().

https://codereview.chromium.org/2624133004/diff/60001/base/timer/timer_unitte...
base/timer/timer_unittest.cc:602: }    // MessageLoop destructs by falling out
of scope. SHOULD NOT CRASH.
Since the goal in this test is to hit ~Timer() before it fires, let's make its
delay much larger than 10ms (with a comment explaining why)

[pkotwicz, 2017-01-16 19:45:52]

gab@ can you please take another look? I think that I have addressed all of your
comments

[gab, 2017-01-17 19:09:45]

lgtm, thanks!

[gab, 2017-01-17 19:09:49]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2017-01-17 19:10:10]

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, 2017-01-17 20:45:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484680189443080,
"parent_rev": "fa882b53462e83fd7a2470f375ee322f7c9c8914", "commit_rev":
"4a286ed6b9406edfb1031ae1d48766f79652a6ca"}

[commit-bot: I haz the power, 2017-01-17 20:46:03]

Description was changed from

==========
Fix use-after-free in base::Timer::StopAndAbandon()

A timer may be owned by a ref counted class. If |Timer::user_task_| is a method
in the ref counted class, it is possible for |Timer::user_task_| to hold the
only reference to the ref counted class. This CL changes
Timer::StopAndAbandon() so that Timer::Stop() is called after
Timer::AbandonScheduledTask(). If |Timer::user_task_| holds the only reference
to ref counted class, Timer::Stop() destroys the timer object.

Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.

BUG=678592
TEST=MessageLoopShutdownSelfOwningTimer
==========

to

==========
Fix use-after-free in base::Timer::StopAndAbandon()

A timer may be owned by a ref counted class. If |Timer::user_task_| is a method
in the ref counted class, it is possible for |Timer::user_task_| to hold the
only reference to the ref counted class. This CL changes
Timer::StopAndAbandon() so that Timer::Stop() is called after
Timer::AbandonScheduledTask(). If |Timer::user_task_| holds the only reference
to ref counted class, Timer::Stop() destroys the timer object.

Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.

BUG=678592
TEST=MessageLoopShutdownSelfOwningTimer

Review-Url: https://codereview.chromium.org/2624133004
Cr-Commit-Position: refs/heads/master@{#444132}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a286ed6b9406edfb1031ae1d487...
==========

[commit-bot: I haz the power, 2017-01-17 20:46:04]

Committed patchset #2 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4a286ed6b9406edfb1031ae1d487...