Bug 1380530 - don't allow new tasks to be posted to MessageLoop when it is shutting down. r=billm
authorLee Salzman <lsalzman@mozilla.com>
Fri, 21 Jul 2017 11:33:53 -0400
changeset 419045 04e3649ca8fc5d0eb8bda167889e01b3c2a68c9b
parent 419044 96941e476121567971b29c4b526494b97e883279
child 419046 cd615bff6069fe4072c67364f2dbb8266a2708c6
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbillm
bugs1380530
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1380530 - don't allow new tasks to be posted to MessageLoop when it is shutting down. r=billm MozReview-Commit-ID: KsXK7UPIO4P
ipc/chromium/src/base/message_loop.cc
ipc/chromium/src/base/message_loop.h
ipc/chromium/src/base/object_watcher.cc
--- a/ipc/chromium/src/base/message_loop.cc
+++ b/ipc/chromium/src/base/message_loop.cc
@@ -172,16 +172,17 @@ static mozilla::Atomic<int32_t> message_
 
 MessageLoop::MessageLoop(Type type, nsIThread* aThread)
     : type_(type),
       id_(++message_loop_id_seq),
       nestable_tasks_allowed_(true),
       exception_restoration_(false),
       state_(NULL),
       run_depth_base_(1),
+      shutting_down_(false),
 #ifdef OS_WIN
       os_modal_loop_(false),
 #endif  // OS_WIN
       transient_hang_timeout_(0),
       permanent_hang_timeout_(0),
       next_sequence_num_(0) {
   DCHECK(!current()) << "should only have one message loop per thread";
   get_tls_ptr().Set(this);
@@ -372,16 +373,19 @@ void MessageLoop::PostTask_Helper(alread
       rv = target->DelayedDispatch(Move(task), delay_ms);
     } else {
       rv = target->Dispatch(Move(task), 0);
     }
     MOZ_ALWAYS_SUCCEEDS(rv);
     return;
   }
 
+  // Tasks should only be queued before or during the Run loop, not after.
+  MOZ_ASSERT(!shutting_down_);
+
 #ifdef MOZ_TASK_TRACER
   nsCOMPtr<nsIRunnable> tracedTask = task;
   if (mozilla::tasktracer::IsStartLogging()) {
     tracedTask = mozilla::tasktracer::CreateTracedRunnable(Move(task));
     (static_cast<mozilla::tasktracer::TracedRunnable*>(tracedTask.get()))->DispatchTask();
   }
   PendingTask pending_task(tracedTask.forget(), true);
 #else
@@ -562,16 +566,19 @@ bool MessageLoop::DoIdleWork() {
 
   return false;
 }
 
 //------------------------------------------------------------------------------
 // MessageLoop::AutoRunState
 
 MessageLoop::AutoRunState::AutoRunState(MessageLoop* loop) : loop_(loop) {
+  // Top-level Run should only get called once.
+  MOZ_ASSERT(!loop_->shutting_down_);
+
   // Make the loop reference us.
   previous_state_ = loop_->state_;
   if (previous_state_) {
     run_depth = previous_state_->run_depth + 1;
   } else {
     run_depth = 1;
   }
   loop_->state_ = this;
@@ -580,16 +587,19 @@ MessageLoop::AutoRunState::AutoRunState(
   quit_received = false;
 #if defined(OS_WIN)
   dispatcher = NULL;
 #endif
 }
 
 MessageLoop::AutoRunState::~AutoRunState() {
   loop_->state_ = previous_state_;
+
+  // If exiting a top-level Run, then we're shutting down.
+  loop_->shutting_down_ = !previous_state_;
 }
 
 //------------------------------------------------------------------------------
 // MessageLoop::PendingTask
 
 bool MessageLoop::PendingTask::operator<(const PendingTask& other) const {
   // Since the top of a priority queue is defined as the "greatest" element, we
   // need to invert the comparison here.  We want the smaller time to be at the
--- a/ipc/chromium/src/base/message_loop.h
+++ b/ipc/chromium/src/base/message_loop.h
@@ -107,19 +107,26 @@ public:
   //
   // The NonNestable variants work similarly except that they promise never to
   // dispatch the task from a nested invocation of MessageLoop::Run.  Instead,
   // such tasks get deferred until the top-most MessageLoop::Run is executing.
   //
   // The MessageLoop takes ownership of the Task, and deletes it after it has
   // been Run().
   //
+  // New tasks should not be posted after the invocation of a MessageLoop's
+  // Run method. Otherwise, they may fail to actually run. Callers should check
+  // if the MessageLoop is processing tasks if necessary by calling
+  // IsAcceptingTasks().
+  //
   // NOTE: These methods may be called on any thread.  The Task will be invoked
   // on the thread that executes MessageLoop::Run().
 
+  bool IsAcceptingTasks() const { return !shutting_down_; }
+
   void PostTask(already_AddRefed<nsIRunnable> task);
 
   void PostDelayedTask(already_AddRefed<nsIRunnable> task, int delay_ms);
 
   // PostIdleTask is not thread safe and should be called on this thread
   void PostIdleTask(already_AddRefed<nsIRunnable> task);
 
   // Run the message loop.
@@ -424,16 +431,17 @@ public:
   // have not yet been sorted out into items for our work_queue_ vs items that
   // will be handled by the TimerManager.
   TaskQueue incoming_queue_;
   // Protect access to incoming_queue_.
   Lock incoming_queue_lock_;
 
   RunState* state_;
   int run_depth_base_;
+  bool shutting_down_;
 
 #if defined(OS_WIN)
   // Should be set to true before calling Windows APIs like TrackPopupMenu, etc
   // which enter a modal message loop.
   bool os_modal_loop_;
 #endif
 
   // Timeout values for hang monitoring
--- a/ipc/chromium/src/base/object_watcher.cc
+++ b/ipc/chromium/src/base/object_watcher.cc
@@ -122,17 +122,19 @@ void CALLBACK ObjectWatcher::DoneWaiting
   RefPtr<Watch> addrefedWatch = watch;
 
   // Record that we ran this function.
   watch->did_signal = true;
 
   // We rely on the locking in PostTask() to ensure that a memory barrier is
   // provided, which in turn ensures our change to did_signal can be observed
   // on the target thread.
-  watch->origin_loop->PostTask(addrefedWatch.forget());
+  if (watch->origin_loop->IsAcceptingTasks()) {
+    watch->origin_loop->PostTask(addrefedWatch.forget());
+  }
 }
 
 void ObjectWatcher::WillDestroyCurrentMessageLoop() {
   // Need to shutdown the watch so that we don't try to access the MessageLoop
   // after this point.
   StopWatching();
 }