Fixed member use after destruction in BarrierClosure.

base/barrier_closure_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/barrier_closure.h"
 
 #include "base/bind.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
 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