Put back r1891 with some task deletion disabled to maintain backwards compat and thereby avoid unexpected crashes.

r=jar

Review URL: http://codereview.chromium.org/1664

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@1959 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 121 insertions, 38 deletions

diff --git a/base/message_loop.cc b/base/message_loop.cc
index ee4a344..122bd06 100644
--- a/base/message_loop.cc
+++ b/base/message_loop.cc
@@ -83,25 +83,27 @@ MessageLoop::~MessageLoop() {
   FOR_EACH_OBSERVER(DestructionObserver, destruction_observers_,
                     WillDestroyCurrentMessageLoop());
 
-  // OK, now make it so that no one can find us.
-  tls_index_.Set(NULL);
-
   DCHECK(!state_);
 
-  // Most tasks that have not been Run() are deleted in the |timer_manager_|
-  // destructor after we remove our tls index.  We delete the tasks in our
-  // queues here so their destuction is similar to the tasks in the
-  // |timer_manager_|.
-  DeletePendingTasks();
-  ReloadWorkQueue();
-  DeletePendingTasks();
-
-  // Delete tasks in the delayed work queue.
-  while (!delayed_work_queue_.empty()) {
-    Task* task = delayed_work_queue_.top().task;
-    delayed_work_queue_.pop();
-    delete task;
+  // Clean up any unprocessed tasks, but take care: deleting a task could
+  // result in the addition of more tasks (e.g., via DeleteSoon).  We set a
+  // limit on the number of times we will allow a deleted task to generate more
+  // tasks.  Normally, we should only pass through this loop once or twice.  If
+  // we end up hitting the loop limit, then it is probably due to one task that
+  // is being stubborn.  Inspect the queues to see who is left.
+  bool did_work;
+  for (int i = 0; i < 100; ++i) {
+    DeletePendingTasks();
+    ReloadWorkQueue();
+    // If we end up with empty queues, then break out of the loop.
+    did_work = DeletePendingTasks();
+    if (!did_work)
+      break;
   }
+  DCHECK(!did_work);
+
+  // OK, now make it so that no one can find us.
+  tls_index_.Set(NULL);
 }
 
 void MessageLoop::AddDestructionObserver(DestructionObserver *obs) {
@@ -290,6 +292,16 @@ bool MessageLoop::DeferOrRunPendingTask(const PendingTask& pending_task) {
   return false;
 }
 
+void MessageLoop::AddToDelayedWorkQueue(const PendingTask& pending_task) {
+  // Move to the delayed work queue.  Initialize the sequence number
+  // before inserting into the delayed_work_queue_.  The sequence number
+  // is used to faciliate FIFO sorting when two tasks have the same
+  // delayed_run_time value.
+  PendingTask new_pending_task(pending_task);
+  new_pending_task.sequence_num = next_sequence_num_++;
+  delayed_work_queue_.push(new_pending_task);
+}
+
 void MessageLoop::ReloadWorkQueue() {
   // We can improve performance of our loading tasks from incoming_queue_ to
   // work_queue_ by waiting until the last minute (work_queue_ is empty) to
@@ -308,20 +320,35 @@ void MessageLoop::ReloadWorkQueue() {
   }
 }
 
-void MessageLoop::DeletePendingTasks() {
-  /* Comment this out as it's causing crashes.
-  while (!work_queue_.Empty()) {
-    Task* task = work_queue_.Pop();
-    if (task->is_owned_by_message_loop())
-      delete task;
+bool MessageLoop::DeletePendingTasks() {
+  bool did_work = !work_queue_.empty();
+  while (!work_queue_.empty()) {
+    PendingTask pending_task = work_queue_.front();
+    work_queue_.pop();
+    if (!pending_task.delayed_run_time.is_null()) {
+      // We want to delete delayed tasks in the same order in which they would
+      // normally be deleted in case of any funny dependencies between delayed
+      // tasks.
+      AddToDelayedWorkQueue(pending_task);
+    } else {
+      // TODO(darin): Delete all tasks once it is safe to do so.
+      //delete task;
+    }
   }
-
-  while (!delayed_non_nestable_queue_.Empty()) {
-    Task* task = delayed_non_nestable_queue_.Pop();
-    if (task->is_owned_by_message_loop())
-      delete task;
+  did_work |= !deferred_non_nestable_work_queue_.empty();
+  while (!deferred_non_nestable_work_queue_.empty()) {
+    Task* task = deferred_non_nestable_work_queue_.front().task;
+    deferred_non_nestable_work_queue_.pop();
+    // TODO(darin): Delete all tasks once it is safe to do so.
+    //delete task;
+  }
+  did_work |= !delayed_work_queue_.empty();
+  while (!delayed_work_queue_.empty()) {
+    Task* task = delayed_work_queue_.top().task;
+    delayed_work_queue_.pop();
+    delete task;
   }
-  */
+  return did_work;
 }
 
 bool MessageLoop::DoWork() {
@@ -341,14 +368,7 @@ bool MessageLoop::DoWork() {
       work_queue_.pop();
       if (!pending_task.delayed_run_time.is_null()) {
         bool was_empty = delayed_work_queue_.empty();
-
-        // Move to the delayed work queue.  Initialize the sequence number
-        // before inserting into the delayed_work_queue_.  The sequence number
-        // is used to faciliate FIFO sorting when two tasks have the same
-        // delayed_run_time value.
-        pending_task.sequence_num = next_sequence_num_++;
-        delayed_work_queue_.push(pending_task);
-
+        AddToDelayedWorkQueue(pending_task);
         if (was_empty)  // We only schedule the next delayed work item.
           pump_->ScheduleDelayedWork(pending_task.delayed_run_time);
       } else {
diff --git a/base/message_loop.h b/base/message_loop.h
index 53c832a..f4511f8 100644
--- a/base/message_loop.h
+++ b/base/message_loop.h
@@ -316,14 +316,18 @@ class MessageLoop : public base::MessagePump::Delegate {
   // cannot be run right now.  Returns true if the task was run.
   bool DeferOrRunPendingTask(const PendingTask& pending_task);
 
+  // Adds the pending task to delayed_work_queue_.
+  void AddToDelayedWorkQueue(const PendingTask& pending_task);
+
   // Load tasks from the incoming_queue_ into work_queue_ if the latter is
   // empty.  The former requires a lock to access, while the latter is directly
   // accessible on this thread.
   void ReloadWorkQueue();
 
   // Delete tasks that haven't run yet without running them.  Used in the
-  // destructor to make sure all the task's destructors get called.
-  void DeletePendingTasks();
+  // destructor to make sure all the task's destructors get called.  Returns
+  // true if some work was done.
+  bool DeletePendingTasks();
 
   // Post a task to our incomming queue.
   void PostTask_Helper(const tracked_objects::Location& from_here, Task* task,
@@ -364,6 +368,7 @@ class MessageLoop : public base::MessagePump::Delegate {
   scoped_refptr<base::MessagePump> pump_;
 
   ObserverList<DestructionObserver> destruction_observers_;
+
   // A recursion block that prevents accidentally running additonal tasks when
   // insider a (accidentally induced?) nested message pump.
   bool nestable_tasks_allowed_;
diff --git a/base/message_loop_unittest.cc b/base/message_loop_unittest.cc
index b36090a6d..e79c576 100644
--- a/base/message_loop_unittest.cc
+++ b/base/message_loop_unittest.cc
@@ -290,6 +290,52 @@ void RunTest_PostDelayedTask_InPostOrder_3(MessageLoop::Type message_loop_type)
   EXPECT_TRUE(run_time2 > run_time1);
 }
 
+class RecordDeletionTask : public Task {
+ public:
+  RecordDeletionTask(Task* post_on_delete, bool* was_deleted)
+      : post_on_delete_(post_on_delete), was_deleted_(was_deleted) {
+  }
+  ~RecordDeletionTask() {
+    *was_deleted_ = true;
+    if (post_on_delete_)
+      MessageLoop::current()->PostTask(FROM_HERE, post_on_delete_);
+  }
+  virtual void Run() {}
+ private:
+  Task* post_on_delete_;
+  bool* was_deleted_;
+};
+
+void RunTest_EnsureTaskDeletion(MessageLoop::Type message_loop_type) {
+  bool a_was_deleted = false;
+  bool b_was_deleted = false;
+  {
+    MessageLoop loop(message_loop_type);
+    loop.PostTask(
+        FROM_HERE, new RecordDeletionTask(NULL, &a_was_deleted));
+    loop.PostDelayedTask(
+        FROM_HERE, new RecordDeletionTask(NULL, &b_was_deleted), 1000);
+  }
+  EXPECT_TRUE(a_was_deleted);
+  EXPECT_TRUE(b_was_deleted);
+}
+
+void RunTest_EnsureTaskDeletion_Chain(MessageLoop::Type message_loop_type) {
+  bool a_was_deleted = false;
+  bool b_was_deleted = false;
+  bool c_was_deleted = false;
+  {
+    MessageLoop loop(message_loop_type);
+    RecordDeletionTask* a = new RecordDeletionTask(NULL, &a_was_deleted);
+    RecordDeletionTask* b = new RecordDeletionTask(a, &b_was_deleted);
+    RecordDeletionTask* c = new RecordDeletionTask(b, &c_was_deleted);
+    loop.PostTask(FROM_HERE, c);
+  }
+  EXPECT_TRUE(a_was_deleted);
+  EXPECT_TRUE(b_was_deleted);
+  EXPECT_TRUE(c_was_deleted);
+}
+
 class NestingTest : public Task {
  public:
   explicit NestingTest(int* depth) : depth_(depth) {
@@ -1049,6 +1095,18 @@ TEST(MessageLoopTest, PostDelayedTask_InPostOrder_3) {
   RunTest_PostDelayedTask_InPostOrder_3(MessageLoop::TYPE_IO);
 }
 
+TEST(MessageLoopTest, EnsureTaskDeletion) {
+  RunTest_EnsureTaskDeletion(MessageLoop::TYPE_DEFAULT);
+  RunTest_EnsureTaskDeletion(MessageLoop::TYPE_UI);
+  RunTest_EnsureTaskDeletion(MessageLoop::TYPE_IO);
+}
+
+TEST(MessageLoopTest, EnsureTaskDeletion_Chain) {
+  RunTest_EnsureTaskDeletion_Chain(MessageLoop::TYPE_DEFAULT);
+  RunTest_EnsureTaskDeletion_Chain(MessageLoop::TYPE_UI);
+  RunTest_EnsureTaskDeletion_Chain(MessageLoop::TYPE_IO);
+}
+
 #if defined(OS_WIN)
 TEST(MessageLoopTest, Crasher) {
   RunTest_Crasher(MessageLoop::TYPE_DEFAULT);