Fixed member use after destruction in BarrierClosure.

Fixed member use after destruction in BarrierClosure.

If client releases BarrierClosure during |done_closure_| invocation, the closure
is destructed together with BarrierInfo object that it owns. After that
BarrierInfo tries to call |done_closure_.Reset()| which leads to SIGSEGV.
Fixed that by saving |done_clusure| to a stack variable.

BUG=none
TEST=base_unittests --gtest_filter=BarrierClosureTest.KeepingClosureAliveUntilDone

Committed: https://crrev.com/0a3f459d5d61443db9989db8201908bf68a684e9
Cr-Commit-Position: refs/heads/master@{#328669}

[dzhioev (left Google), 2015-04-30 02:16:13]

dzhioev@chromium.org changed reviewers:
+ danakj@chromium.org

[dzhioev (left Google), 2015-04-30 02:18:39]

Hello, please review.

[danakj, 2015-04-30 17:32:56]

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure.cc
File base/barrier_closure.cc (right):

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure.cc#new...
base/barrier_closure.cc:31: base::Closure done_closure = done_closure_;
Why no Reset still? Was it important before? There appears to be no test
coverage for that Reset.

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure_unitte...
File base/barrier_closure_unittest.cc (right):

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure_unitte...
base/barrier_closure_unittest.cc:26: base::Closure barier_closure =
base::BarrierClosure(2, done_closure);
barrier

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure_unitte...
base/barrier_closure_unittest.cc:45: barier_closure = base::BarrierClosure(1,
done_closure);
BarrierClosure makes a copy of done_closure in the constructor. So changing
done_closure after this function should not affect barrier closure?

I don't understand how BarrierClosure is doing something wrong, can you explain
further WRT this test?

[dzhioev (left Google), 2015-04-30 20:00:54]

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure.cc
File base/barrier_closure.cc (right):

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure.cc#new...
base/barrier_closure.cc:31: base::Closure done_closure = done_closure_;
On 2015/04/30 17:32:55, danakj wrote:
> Why no Reset still? Was it important before? There appears to be no test
> coverage for that Reset.

I missed that part. Returned Reset back and added a test case.

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure_unitte...
File base/barrier_closure_unittest.cc (right):

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure_unitte...
base/barrier_closure_unittest.cc:26: base::Closure barier_closure =
base::BarrierClosure(2, done_closure);
On 2015/04/30 17:32:55, danakj wrote:
> barrier

Done.

https://codereview.chromium.org/1116833002/diff/1/base/barrier_closure_unitte...
base/barrier_closure_unittest.cc:45: barier_closure = base::BarrierClosure(1,
done_closure);
On 2015/04/30 17:32:55, danakj wrote:
> BarrierClosure makes a copy of done_closure in the constructor. So changing
> done_closure after this function should not affect barrier closure?
> 
> I don't understand how BarrierClosure is doing something wrong, can you
explain
> further WRT this test?

I change |barrier_closure|, not |done_closure|.
Detailed explanation of the problem:
1. base::BarrierClosure creates a |barrier_closure|, which owns a BarrierInfo
object, which has a member |done_closure_|, which is a copy of |done_closure|.
2. When we call Run on |barrier_closure|, BarrierInfo calls Run() on 
|done_closure_| in BarrierInfo::Run().
3. |done_closure| resets |barrier_closure|. Since |barier_closure| is a last and
only copy, BarrierInfo object is being destructed.
4. Control returns to BarrierInfo::Run(), where |done_closure_.Reset()| is
called. But at that moment |this| is already destructed, so |done_closure_|
contains a garbage.

[danakj, 2015-05-05 18:58:14]

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
File base/barrier_closure_unittest.cc (right):

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
base/barrier_closure_unittest.cc:56: CHECK(!done_destructed);
Use EXPECT_TRUE/EXPECT_FALSE instead of CHECK in tests

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
base/barrier_closure_unittest.cc:61: void ReleaseClosure(base::Closure* closure)
{
can you call this ResetBarrierClosure?

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
base/barrier_closure_unittest.cc:65: // Tests a case when client releases a
reference to barrierClosure in
BarrierClosure

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
base/barrier_closure_unittest.cc:70: barrier_closure = base::BarrierClosure(1,
done_closure);
Can you add a more verbose comment explaining what this test is testing when it
calls Run() since it has no EXPECTations? Maybe like

// barrier_closure is a Closure holding a done_closure.
// done_closure holds a pointer back to the barrier_closure.
// When barrier_closure is Run() it calls ResetBarrierClosure() which
// erases the barrier_closure while still inside of its Run(). The
// Run() implementation (in base::BarrierClosure) must not try use
// itself after executing ResetBarrierClosure() or this test would
// crash inside Run().

[dzhioev (left Google), 2015-05-06 23:13:05]

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
File base/barrier_closure_unittest.cc (right):

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
base/barrier_closure_unittest.cc:56: CHECK(!done_destructed);
On 2015/05/05 18:58:14, danakj wrote:
> Use EXPECT_TRUE/EXPECT_FALSE instead of CHECK in tests

Done.

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
base/barrier_closure_unittest.cc:61: void ReleaseClosure(base::Closure* closure)
{
On 2015/05/05 18:58:14, danakj wrote:
> can you call this ResetBarrierClosure?

Done.

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
base/barrier_closure_unittest.cc:65: // Tests a case when client releases a
reference to barrierClosure in
On 2015/05/05 18:58:14, danakj wrote:
> BarrierClosure

Done.

https://codereview.chromium.org/1116833002/diff/40001/base/barrier_closure_un...
base/barrier_closure_unittest.cc:70: barrier_closure = base::BarrierClosure(1,
done_closure);
On 2015/05/05 18:58:14, danakj wrote:
> Can you add a more verbose comment explaining what this test is testing when
it
> calls Run() since it has no EXPECTations? Maybe like
> 
> // barrier_closure is a Closure holding a done_closure.
> // done_closure holds a pointer back to the barrier_closure.
> // When barrier_closure is Run() it calls ResetBarrierClosure() which
> // erases the barrier_closure while still inside of its Run(). The
> // Run() implementation (in base::BarrierClosure) must not try use
> // itself after executing ResetBarrierClosure() or this test would
> // crash inside Run().

Done.

[danakj, 2015-05-06 23:15:10]

LGTM

[dzhioev (left Google), 2015-05-06 23:24:36]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2015-05-06 23:25:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1116833002/60001

[commit-bot: I haz the power, 2015-05-07 01:05:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-07 01:06:30]

Patchset 4 (id:??) landed as
https://crrev.com/0a3f459d5d61443db9989db8201908bf68a684e9
Cr-Commit-Position: refs/heads/master@{#328669}