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

Review URL: https://codereview.chromium.org/1116833002

Cr-Commit-Position: refs/heads/master@{#328669}

base/barrier_closure.cc

@@ -28,8 +28,9 @@ BarrierInfo::BarrierInfo(int num_callbacks, const base::Closure& done_closure)
 void BarrierInfo::Run() {
   DCHECK(!base::AtomicRefCountIsZero(&num_callbacks_left_));
   if (!base::AtomicRefCountDec(&num_callbacks_left_)) {
-    done_closure_.Run();
+    base::Closure done_closure = done_closure_;
     done_closure_.Reset();
+    done_closure.Run();
   }
 }
 

base/barrier_closure_unittest.cc

@@ -13,24 +13,68 @@ 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))));
+  EXPECT_FALSE(done_destructed);
+  barrier_closure.Run();
+  EXPECT_TRUE(done_destructed);
+}
+
+void ResetBarrierClosure(base::Closure* closure) {
+  *closure = base::Closure();
+}
+
+// Tests a case when |done_closure| resets a |barrier_closure|.
+// |barrier_closure| is a Closure holding the |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().
+TEST(BarrierClosureTest, KeepingClosureAliveUntilDone) {
+  base::Closure barrier_closure;
+  base::Closure done_closure =
+      base::Bind(ResetBarrierClosure, &barrier_closure);
+  barrier_closure = base::BarrierClosure(1, done_closure);
+  barrier_closure.Run();
+}
+
 }  // namespace