Bug 1507093 - P1. Ensure we never leak OwnerType object. r=gerald

Potentially, if the watcher notification task failed to dispatch, we would have a cycle left between the WatchManager and the OwnerType

Differential Revision: https://phabricator.services.mozilla.com/D11857

--HG--
extra : moz-landing-system : lando

xpcom/threads/StateWatching.h

@@ -59,7 +59,7 @@ namespace mozilla {
 
 extern LazyLogModule gStateWatchingLog;
 
-#define WATCH_LOG(x, ...) \
+#define WATCH_LOG(x, ...)                                                      \
   MOZ_LOG(gStateWatchingLog, LogLevel::Debug, (x, ##__VA_ARGS__))
 
 /*
@@ -69,7 +69,10 @@ class AbstractWatcher
 {
 public:
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AbstractWatcher)
-  AbstractWatcher() : mDestroyed(false) {}
+  AbstractWatcher()
+    : mDestroyed(false)
+  {
+  }
   bool IsDestroyed() { return mDestroyed; }
   virtual void Notify() = 0;
 
@@ -89,7 +92,10 @@ class AbstractWatcher
 class WatchTarget
 {
 public:
-  explicit WatchTarget(const char* aName) : mName(aName) {}
+  explicit WatchTarget(const char* aName)
+    : mName(aName)
+  {
+  }
 
   void AddWatcher(AbstractWatcher* aWatcher)
   {
@@ -141,7 +147,10 @@ class Watchable : public WatchTarget
 {
 public:
   Watchable(const T& aInitialValue, const char* aName)
-    : WatchTarget(aName), mValue(aInitialValue) {}
+    : WatchTarget(aName)
+    , mValue(aInitialValue)
+  {
+  }
 
   const T& Ref() const { return mValue; }
   operator const T&() const { return Ref(); }
@@ -156,8 +165,8 @@ class Watchable : public WatchTarget
   }
 
 private:
-  Watchable(const Watchable& aOther); // Not implemented
-  Watchable& operator=(const Watchable& aOther); // Not implemented
+  Watchable(const Watchable& aOther) = delete;
+  Watchable& operator=(const Watchable& aOther) = delete;
 
   T mValue;
 };
@@ -167,25 +176,28 @@ class Watchable : public WatchTarget
 //
 // Internally, WatchManager maintains one AbstractWatcher per callback method.
 // Consumers invoke Watch/Unwatch on a particular (WatchTarget, Callback) tuple.
-// This causes an AbstractWatcher for |Callback| to be instantiated if it doesn't
-// already exist, and registers it with |WatchTarget|.
+// This causes an AbstractWatcher for |Callback| to be instantiated if it
+// doesn't already exist, and registers it with |WatchTarget|.
 //
 // Using Direct Tasks on the TailDispatcher, WatchManager ensures that we fire
-// watch callbacks no more than once per task, once all other operations for that
-// task have been completed.
+// watch callbacks no more than once per task, once all other operations for
+// that task have been completed.
 //
 // WatchManager<OwnerType> is intended to be declared as a member of |OwnerType|
-// objects. Given that, it and its owned objects can't hold permanent strong refs to
-// the owner, since that would keep the owner alive indefinitely. Instead, it
-// _only_ holds strong refs while waiting for Direct Tasks to fire. This ensures
-// that everything is kept alive just long enough.
-template <typename OwnerType>
+// objects. Given that, it and its owned objects can't hold permanent strong
+// refs to the owner, since that would keep the owner alive indefinitely.
+// Instead, it _only_ holds strong refs while waiting for Direct Tasks to fire.
+// This ensures that everything is kept alive just long enough.
+template<typename OwnerType>
 class WatchManager
 {
 public:
-  typedef void(OwnerType::*CallbackMethod)();
+  typedef void (OwnerType::*CallbackMethod)();
   explicit WatchManager(OwnerType* aOwner, AbstractThread* aOwnerThread)
-    : mOwner(aOwner), mOwnerThread(aOwnerThread) {}
+    : mOwner(aOwner)
+    , mOwnerThread(aOwnerThread)
+  {
+  }
 
   ~WatchManager()
   {
@@ -201,8 +213,8 @@ class WatchManager
   void Shutdown()
   {
     MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
-    for (size_t i = 0; i < mWatchers.Length(); ++i) {
-      mWatchers[i]->Destroy();
+    for (auto& watcher : mWatchers) {
+      watcher->Destroy();
     }
     mWatchers.Clear();
     mOwner = nullptr;
@@ -234,8 +246,14 @@ class WatchManager
   class PerCallbackWatcher : public AbstractWatcher
   {
   public:
-    PerCallbackWatcher(OwnerType* aOwner, AbstractThread* aOwnerThread, CallbackMethod aMethod)
-      : mOwner(aOwner), mOwnerThread(aOwnerThread), mCallbackMethod(aMethod) {}
+    PerCallbackWatcher(OwnerType* aOwner,
+                       AbstractThread* aOwnerThread,
+                       CallbackMethod aMethod)
+      : mOwner(aOwner)
+      , mOwnerThread(aOwnerThread)
+      , mCallbackMethod(aMethod)
+    {
+    }
 
     void Destroy()
     {
@@ -247,19 +265,26 @@ class WatchManager
     void Notify() override
     {
       MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
-      MOZ_DIAGNOSTIC_ASSERT(mOwner, "mOwner is only null after destruction, "
-                                    "at which point we shouldn't be notified");
-      if (mStrongRef) {
+      MOZ_DIAGNOSTIC_ASSERT(mOwner,
+                            "mOwner is only null after destruction, "
+                            "at which point we shouldn't be notified");
+      if (mNotificationPending) {
         // We've already got a notification job in the pipe.
         return;
       }
-      mStrongRef = mOwner; // Hold the owner alive while notifying.
+      mNotificationPending = true;
 
       // Queue up our notification jobs to run in a stable state.
       mOwnerThread->TailDispatcher().AddDirectTask(
-        NewRunnableMethod("WatchManager::PerCallbackWatcher::DoNotify",
-                          this,
-                          &PerCallbackWatcher::DoNotify));
+        NS_NewRunnableFunction("WatchManager::PerCallbackWatcher::Notify", [
+          self = RefPtr<PerCallbackWatcher>(this),
+          owner = RefPtr<OwnerType>(mOwner)
+        ]() {
+          if (!self->mDestroyed) {
+            ((*owner).*(self->mCallbackMethod))();
+          }
+          self->mNotificationPending = false;
+        }));
     }
 
     bool CallbackMethodIs(CallbackMethod aMethod) const
@@ -268,30 +293,20 @@ class WatchManager
     }
 
   private:
-    ~PerCallbackWatcher() {}
-
-    void DoNotify()
-    {
-      MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
-      MOZ_ASSERT(mStrongRef);
-      RefPtr<OwnerType> ref = mStrongRef.forget();
-      if (!mDestroyed) {
-        ((*ref).*mCallbackMethod)();
-      }
-    }
+    ~PerCallbackWatcher() = default;
 
     OwnerType* mOwner; // Never null.
-    RefPtr<OwnerType> mStrongRef; // Only non-null when notifying.
+    bool mNotificationPending = false;
     RefPtr<AbstractThread> mOwnerThread;
     CallbackMethod mCallbackMethod;
   };
 
   PerCallbackWatcher* GetWatcher(CallbackMethod aMethod)
   {
     MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
-    for (size_t i = 0; i < mWatchers.Length(); ++i) {
-      if (mWatchers[i]->CallbackMethodIs(aMethod)) {
-        return mWatchers[i];
+    for (auto& watcher : mWatchers) {
+      if (watcher->CallbackMethodIs(aMethod)) {
+        return watcher;
       }
     }
     return nullptr;
@@ -304,7 +319,10 @@ class WatchManager
     if (watcher) {
       return *watcher;
     }
-    watcher = mWatchers.AppendElement(new PerCallbackWatcher(mOwner, mOwnerThread, aMethod))->get();
+    watcher = mWatchers
+                .AppendElement(MakeAndAddRef<PerCallbackWatcher>(
+                  mOwner, mOwnerThread, aMethod))
+                ->get();
     return *watcher;
   }