Bug 1380619 - Avoid unnecessary content process leaks in SchedulerGroup dispatch during shutdown. r=mystor, a=lizzard
authorNathan Froyd <froydnj@mozilla.com>
Wed, 09 Aug 2017 14:12:44 -0400
changeset 423568 572e7636edd17841d951f842f52184888138f163
parent 423567 7ad9e8f27a03af75685299afa5a6a6b3c7777ba4
child 423569 690335e86039d4501232c26aacef86862e89edc2
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmystor, lizzard
bugs1380619
milestone56.0
Bug 1380619 - Avoid unnecessary content process leaks in SchedulerGroup dispatch during shutdown. r=mystor, a=lizzard SchedulerGroup dispatch needs to replicate all the quirks of dispatching directly to threads, which means we need to handle cases where dispatch might have failed and we have resources that we don't want to leak.
xpcom/threads/SchedulerGroup.cpp
xpcom/threads/SchedulerGroup.h
--- a/xpcom/threads/SchedulerGroup.cpp
+++ b/xpcom/threads/SchedulerGroup.cpp
@@ -5,16 +5,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/SchedulerGroup.h"
 
 #include "jsfriendapi.h"
 #include "mozilla/AbstractThread.h"
 #include "mozilla/Atomics.h"
 #include "mozilla/Move.h"
+#include "mozilla/Unused.h"
 #include "nsINamed.h"
 #include "nsQueryObject.h"
 #include "mozilla/dom/ScriptSettings.h"
 #include "nsThreadUtils.h"
 
 #include "mozilla/Telemetry.h"
 
 using namespace mozilla;
@@ -297,21 +298,51 @@ SchedulerGroup::FromEventTarget(nsIEvent
 }
 
 nsresult
 SchedulerGroup::LabeledDispatch(TaskCategory aCategory,
                                 already_AddRefed<nsIRunnable>&& aRunnable)
 {
   nsCOMPtr<nsIRunnable> runnable(aRunnable);
   if (XRE_IsContentProcess()) {
-    runnable = new Runnable(runnable.forget(), this);
+    RefPtr<Runnable> internalRunnable = new Runnable(runnable.forget(), this);
+    return InternalUnlabeledDispatch(aCategory, internalRunnable.forget());
   }
   return UnlabeledDispatch(aCategory, runnable.forget());
 }
 
+/*static*/ nsresult
+SchedulerGroup::InternalUnlabeledDispatch(TaskCategory aCategory,
+                                          already_AddRefed<Runnable>&& aRunnable)
+{
+  if (NS_IsMainThread()) {
+    // NS_DispatchToCurrentThread will not leak the passed in runnable
+    // when it fails, so we don't need to do anything special.
+    return NS_DispatchToCurrentThread(Move(aRunnable));
+  }
+
+  RefPtr<Runnable> runnable(aRunnable);
+  nsresult rv = NS_DispatchToMainThread(do_AddRef(runnable));
+  if (NS_FAILED(rv)) {
+    // Dispatch failed.  This is a situation where we would have used
+    // NS_DispatchToMainThread rather than calling into the SchedulerGroup
+    // machinery, and the caller would be expecting to leak the nsIRunnable
+    // originally passed in.  But because we've had to wrap things up
+    // internally, we were going to leak the nsIRunnable *and* our Runnable
+    // wrapper.  But there's no reason that we have to leak our Runnable
+    // wrapper; we can just leak the wrapped nsIRunnable, and let the caller
+    // take care of unleaking it if they need to.
+    Unused << runnable->mRunnable.forget().take();
+    nsrefcnt refcnt = runnable.get()->Release();
+    MOZ_RELEASE_ASSERT(refcnt == 1, "still holding an unexpected reference!");
+  }
+
+  return rv;
+}
+
 void
 SchedulerGroup::SetValidatingAccess(ValidationType aType)
 {
   sRunningDispatcher = aType == StartValidation ? this : nullptr;
   mAccessValid = aType == StartValidation;
 
   dom::AutoJSAPI jsapi;
   jsapi.Init();
--- a/xpcom/threads/SchedulerGroup.h
+++ b/xpcom/threads/SchedulerGroup.h
@@ -94,16 +94,18 @@ public:
     bool IsBackground() const { return mGroup->IsBackground(); }
 
     NS_DECL_ISUPPORTS_INHERITED
     NS_DECL_NSIRUNNABLE
 
     NS_DECLARE_STATIC_IID_ACCESSOR(NS_SCHEDULERGROUPRUNNABLE_IID);
 
  private:
+    friend class SchedulerGroup;
+
     ~Runnable() = default;
 
     nsCOMPtr<nsIRunnable> mRunnable;
     RefPtr<SchedulerGroup> mGroup;
   };
   friend class Runnable;
 
   bool* GetValidAccessPtr() { return &mAccessValid; }
@@ -124,16 +126,19 @@ public:
   static nsresult UnlabeledDispatch(TaskCategory aCategory,
                                     already_AddRefed<nsIRunnable>&& aRunnable);
 
   static void MarkVsyncReceived();
 
   static void MarkVsyncRan();
 
 protected:
+  static nsresult InternalUnlabeledDispatch(TaskCategory aCategory,
+                                            already_AddRefed<Runnable>&& aRunnable);
+
   // Implementations are guaranteed that this method is called on the main
   // thread.
   virtual AbstractThread* AbstractMainThreadForImpl(TaskCategory aCategory);
 
   // Helper method to create an event target specific to a particular TaskCategory.
   virtual already_AddRefed<nsISerialEventTarget>
   CreateEventTargetFor(TaskCategory aCategory);