Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj
☠☠ backed out by 00a507c0f662 ☠ ☠
authorXidorn Quan <quanxunzhen@gmail.com>
Tue, 29 Sep 2015 09:28:22 +1000
changeset 264806 d8f740ef24304bb4088be28ba1c0204962aeadf1
parent 264805 edc0b56d81faf04f67462221aaaa566319df8c39
child 264807 c6142b815de04b37c1b798b79729dbdb4e827330
push id65754
push userxquan@mozilla.com
push dateMon, 28 Sep 2015 23:28:58 +0000
treeherdermozilla-inbound@c6142b815de0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1186745
milestone44.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 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj
xpcom/threads/nsIEventTarget.idl
xpcom/threads/nsThread.cpp
--- a/xpcom/threads/nsIEventTarget.idl
+++ b/xpcom/threads/nsIEventTarget.idl
@@ -55,17 +55,20 @@ interface nsIEventTarget : nsISupports
    */
   boolean isOnCurrentThread();
 
   /**
    * Dispatch an event to this event target.  This function may be called from
    * any thread, and it may be called re-entrantly.
    *
    * @param event
-   *   The alreadyAddRefed<> event to dispatch
+   *   The alreadyAddRefed<> event to dispatch.
+   *   NOTE that the event will be leaked if it fails to dispatch. Also note
+   *   that if "flags" includes DISPATCH_SYNC, it may return error from Run()
+   *   after a successful dispatch. In that case, the event is not leaked.
    * @param flags
    *   The flags modifying event dispatch.  The flags are described in detail
    *   below.
    *
    * @throws NS_ERROR_INVALID_ARG
    *   Indicates that event is null.
    * @throws NS_ERROR_UNEXPECTED
    *   Indicates that the thread is shutting down and has finished processing
--- a/xpcom/threads/nsThread.cpp
+++ b/xpcom/threads/nsThread.cpp
@@ -576,59 +576,68 @@ nsThread::PutEvent(already_AddRefed<nsIR
 
   return NS_OK;
 }
 
 nsresult
 nsThread::DispatchInternal(already_AddRefed<nsIRunnable>&& aEvent, uint32_t aFlags,
                            nsNestedEventTarget* aTarget)
 {
-  nsCOMPtr<nsIRunnable> event(aEvent);
+  // We want to leak the reference when we fail to dispatch it, so that
+  // we won't release the event in a wrong thread.
+  LeakRefPtr<nsIRunnable> event(Move(aEvent));
   if (NS_WARN_IF(!event)) {
     return NS_ERROR_INVALID_ARG;
   }
 
   if (gXPCOMThreadsShutDown && MAIN_THREAD != mIsMainThread && !aTarget) {
     NS_ASSERTION(false, "Failed Dispatch after xpcom-shutdown-threads");
     return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
   }
 
 #ifdef MOZ_TASK_TRACER
-  nsCOMPtr<nsIRunnable> tracedRunnable = CreateTracedRunnable(event.forget());
+  nsCOMPtr<nsIRunnable> tracedRunnable = CreateTracedRunnable(event.take());
   (static_cast<TracedRunnable*>(tracedRunnable.get()))->DispatchTask();
+  // XXX tracedRunnable will always leaked when we fail to disptch.
   event = tracedRunnable.forget();
 #endif
 
   if (aFlags & DISPATCH_SYNC) {
     nsThread* thread = nsThreadManager::get()->GetCurrentThread();
     if (NS_WARN_IF(!thread)) {
       return NS_ERROR_NOT_AVAILABLE;
     }
 
     // XXX we should be able to do something better here... we should
     //     be able to monitor the slot occupied by this event and use
     //     that to tell us when the event has been processed.
 
     nsRefPtr<nsThreadSyncDispatch> wrapper =
-      new nsThreadSyncDispatch(thread, event.forget());
+      new nsThreadSyncDispatch(thread, event.take());
     nsresult rv = PutEvent(wrapper, aTarget); // hold a ref
     // Don't wait for the event to finish if we didn't dispatch it...
     if (NS_FAILED(rv)) {
+      // PutEvent leaked the wrapper runnable object on failure, so we
+      // explicitly release this object once for that. Note that this
+      // object will be released again soon because it exits the scope.
+      wrapper.get()->Release();
       return rv;
     }
 
     // Allows waiting; ensure no locks are held that would deadlock us!
     while (wrapper->IsPending()) {
       NS_ProcessNextEvent(thread, true);
     }
+    // NOTE that, unlike the behavior above, the event is not leaked by
+    // this place, while it is possible that the result is an error.
     return wrapper->Result();
   }
 
   NS_ASSERTION(aFlags == NS_DISPATCH_NORMAL, "unexpected dispatch flags");
-  return PutEvent(event.forget(), aTarget);
+  return PutEvent(event.take(), aTarget);
 }
 
 //-----------------------------------------------------------------------------
 // nsIEventTarget
 
 NS_IMETHODIMP
 nsThread::DispatchFromScript(nsIRunnable* aEvent, uint32_t aFlags)
 {