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..6ddf60ad042c95c83abdea98856f3db6896f2036 100644
--- a/base/barrier_closure_unittest.cc
+++ b/base/barrier_closure_unittest.cc
@@ -13,24 +13,62 @@ 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 barrier_closure = base::BarrierClosure(2, done_closure);
   EXPECT_EQ(0, count);
 
-  barrierClosure.Run();
+  barrier_closure.Run();
   EXPECT_EQ(0, count);
 
-  barrierClosure.Run();
+  barrier_closure.Run();
   EXPECT_EQ(1, count);
 }
 
+class DestructionIndicator {
+ public:
+  // Sets |*destructed| to true in destructor.
+  DestructionIndicator(bool* destructed) : destructed_(destructed) {
+    *destructed_ = false;
+  }
+
+  ~DestructionIndicator() { *destructed_ = true; }
+
+  void DoNothing() {}
+
+ private:
+  bool* destructed_;
+};
+
+TEST(BarrierClosureTest, ReleasesDoneClosureWhenDone) {
+  bool done_destructed = false;
+  base::Closure barrier_closure = base::BarrierClosure(
+      1, base::Bind(&DestructionIndicator::DoNothing,
+                    base::Owned(new DestructionIndicator(&done_destructed))));
+  CHECK(!done_destructed);

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

Use EXPECT_TRUE/EXPECT_FALSE instead of CHECK in tests

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

Done.

+  barrier_closure.Run();
+  CHECK(done_destructed);
+}
+
+void ReleaseClosure(base::Closure* closure) {

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

can you call this ResetBarrierClosure?

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

Done.

+  *closure = base::Closure();
+}
+
+// Tests a case when client releases a reference to barrierClosure in

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

BarrierClosure

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

Done.

+// |done_closure|.
+TEST(BarrierClosureTest, KeepingClosureAliveUntilDone) {
+  base::Closure barrier_closure;
+  base::Closure done_closure = base::Bind(ReleaseClosure, &barrier_closure);
+  barrier_closure = base::BarrierClosure(1, done_closure);

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

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:04]

Done.

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