Fix a leak in OneShotEvent class.

Once Signal()ed, OneShotEvent should drop all the references to
the tasks, otherwise OneShotEvent::tasks_ would continue to hold
references to users of OneShotEvent::Post(), for the lifetime
of OneShotEvent.

This CL clears |tasks_| once Signal() is called, dropping those
references correctly. In future, OneShotEvent::Post() will take
OnceClosure instead of (repeating) Closure.

This CL also adds a regression test for the same.

Bug: 853649
Change-Id: I10270318ff8393a9af0b8da299a1866df936b273
Reviewed-on: https://chromium-review.googlesource.com/1103606
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568184}

extensions/common/one_shot_event.cc

@@ -72,15 +72,19 @@ void OneShotEvent::Signal() {
   // could proceed immediately, but the fact that this object is
   // single-threaded prevents that from being relevant.
 
-  // We could randomize tasks_ in debug mode in order to check that
+  // Move tasks to a temporary to ensure no new ones are added.
+  std::vector<TaskInfo> moved_tasks;
+  std::swap(moved_tasks, tasks_);
+
+  // We could randomize tasks in debug mode in order to check that
   // the order doesn't matter...
-  for (size_t i = 0; i < tasks_.size(); ++i) {
-    const TaskInfo& task = tasks_[i];
+  for (const TaskInfo& task : moved_tasks) {
     if (task.delay.is_zero())
       task.runner->PostTask(task.from_here, task.task);
     else
       task.runner->PostDelayedTask(task.from_here, task.task, task.delay);
   }
+  DCHECK(tasks_.empty()) << "No new tasks should be added during task running!";
 }
 
 void OneShotEvent::PostImpl(const base::Location& from_here,

extensions/common/one_shot_event_unittest.cc

@@ -17,6 +17,29 @@ namespace {
 
 void Increment(int* i) { ++*i; }
 
+// |*did_delete_instance| will be set to true upon its destruction.
+class RefCountedClass : public base::RefCounted<RefCountedClass> {
+ public:
+  explicit RefCountedClass(bool* did_delete_instance)
+      : did_delete_instance_(did_delete_instance) {
+    DCHECK(!*did_delete_instance_);
+  }
+
+  void PerformTask() { did_perform_task_ = true; }
+  bool did_perform_task() const { return did_perform_task_; }
+
+ private:
+  friend class base::RefCounted<RefCountedClass>;
+
+  ~RefCountedClass() { *did_delete_instance_ = true; }
+
+  bool* const did_delete_instance_;  // Not owned.
+
+  bool did_perform_task_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(RefCountedClass);
+};
+
 TEST(OneShotEventTest, RecordsSignal) {
   OneShotEvent event;
   EXPECT_FALSE(event.is_signaled());
@@ -124,5 +147,29 @@ TEST(OneShotEventTest, IsSignaledAndPostsFromCallbackWork) {
   EXPECT_EQ(1, i);
 }
 
+// Tests that OneShotEvent does not keep references to tasks once OneShotEvent
+// Signal()s.
+TEST(OneShotEventTest, DropsCallbackRefUponSignalled) {
+  auto runner = base::MakeRefCounted<base::TestSimpleTaskRunner>();
+  bool did_delete_instance = false;
+  OneShotEvent event;
+
+  {
+    auto ref_counted_class =
+        base::MakeRefCounted<RefCountedClass>(&did_delete_instance);
+    event.Post(
+        FROM_HERE,
+        base::BindRepeating(&RefCountedClass::PerformTask, ref_counted_class),
+        runner);
+    event.Signal();
+    runner->RunPendingTasks();
+    EXPECT_TRUE(ref_counted_class->did_perform_task());
+  }
+
+  // Once OneShotEvent doesn't have any queued events, it should have dropped
+  // all the references to the callbacks it received through Post().
+  EXPECT_TRUE(did_delete_instance);
+}
+
 }  // namespace
 }  // namespace extensions