Fixed member use after destruction in BarrierClosure.

base/barrier_closure_unittest.cc

Index: base/barrier_closure_unittest.cc
diff --git a/base/barrier_closure_unittest.cc b/base/barrier_closure_unittest.cc
index ab05cb8af540f82f53420a7df47b99fdc0685d39..caa000ff03811048482ba220af4216b1c6142a19 100644
--- a/base/barrier_closure_unittest.cc
+++ b/base/barrier_closure_unittest.cc
@@ -13,24 +13,37 @@ void Increment(int* count) { (*count)++; }
 
 TEST(BarrierClosureTest, RunImmediatelyForZeroClosures) {
   int count = 0;
-  base::Closure doneClosure(base::Bind(&Increment, base::Unretained(&count)));
+  base::Closure done_closure(base::Bind(&Increment, base::Unretained(&count)));
 
-  base::Closure barrierClosure = base::BarrierClosure(0, doneClosure);
+  base::Closure barrier_closure = base::BarrierClosure(0, done_closure);
   EXPECT_EQ(1, count);
 }
 
 TEST(BarrierClosureTest, RunAfterNumClosures) {
   int count = 0;
-  base::Closure doneClosure(base::Bind(&Increment, base::Unretained(&count)));
+  base::Closure done_closure(base::Bind(&Increment, base::Unretained(&count)));
 
-  base::Closure barrierClosure = base::BarrierClosure(2, doneClosure);
+  base::Closure barier_closure = base::BarrierClosure(2, done_closure);

[danakj, 2015/04/30 17:32:55]

barrier

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

Done.

   EXPECT_EQ(0, count);
 
-  barrierClosure.Run();
+  barier_closure.Run();
   EXPECT_EQ(0, count);
 
-  barrierClosure.Run();
+  barier_closure.Run();
   EXPECT_EQ(1, count);
 }
 
+void ReleaseClosure(base::Closure* closure) {
+  *closure = base::Closure();
+}
+
+// Tests a case when client releases a reference to BarierClosure in
+// |done_closure|.
+TEST(BarrierClosureTest, KeepingClosureAliveUntilDone) {
+  base::Closure barier_closure;
+  base::Closure done_closure = base::Bind(ReleaseClosure, &barier_closure);
+  barier_closure = base::BarrierClosure(1, done_closure);

[danakj, 2015/04/30 17:32:55]

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]

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.

+  barier_closure.Run();
+}
+
 }  // namespace