Bug 1196079 - Always try to release Notification via normal WorkerRunnable first. r=wchen

--HG--
extra : commitid : 9vxblA1WA3i
extra : rebase_source : 86f5c81d20d55a41f7e6bb47fe5dc7df8e8504b1

dom/notification/Notification.cpp

@@ -409,6 +409,22 @@ public:
   }
 };
 
+class ReleaseNotificationRunnable final : public NotificationWorkerRunnable
+{
+  Notification* mNotification;
+public:
+  explicit ReleaseNotificationRunnable(Notification* aNotification)
+    : NotificationWorkerRunnable(aNotification->mWorkerPrivate)
+    , mNotification(aNotification)
+  {}
+
+  void
+  WorkerRunInternal(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
+  {
+    mNotification->ReleaseObject();
+  }
+};
+
 // Create one whenever you require ownership of the notification. Use with
 // UniquePtr<>. See Notification.h for details.
 class NotificationRef final {
@@ -452,18 +468,31 @@ public:
   ~NotificationRef()
   {
     if (Initialized() && mNotification) {
-      if (mNotification->mWorkerPrivate && NS_IsMainThread()) {
-        nsRefPtr<ReleaseNotificationControlRunnable> r =
-          new ReleaseNotificationControlRunnable(mNotification);
-        AutoSafeJSContext cx;
-        if (!r->Dispatch(cx)) {
-          MOZ_CRASH("Will leak worker thread Notification!");
+      Notification* notification = mNotification;
+      mNotification = nullptr;
+      if (notification->mWorkerPrivate && NS_IsMainThread()) {
+        // Try to pass ownership back to the worker. If the dispatch succeeds we
+        // are guaranteed this runnable will run, and that it will run after queued
+        // event runnables, so event runnables will have a safe pointer to the
+        // Notification.
+        //
+        // If the dispatch fails, the worker isn't running anymore and the event
+        // runnables have already run or been canceled. We can use a control
+        // runnable to release the reference.
+        nsRefPtr<ReleaseNotificationRunnable> r =
+          new ReleaseNotificationRunnable(notification);
+
+        AutoJSAPI jsapi;
+        jsapi.Init();
+        if (!r->Dispatch(jsapi.cx())) {
+          nsRefPtr<ReleaseNotificationControlRunnable> r =
+            new ReleaseNotificationControlRunnable(notification);
+          MOZ_ALWAYS_TRUE(r->Dispatch(jsapi.cx()));
         }
       } else {
-        mNotification->AssertIsOnTargetThread();
-        mNotification->ReleaseObject();
+        notification->AssertIsOnTargetThread();
+        notification->ReleaseObject();
       }
-      mNotification = nullptr;
     }
   }
 
@@ -973,44 +1002,6 @@ protected:
     AssertIsOnMainThread();
 
     MOZ_ASSERT(mNotificationRef);
-    Notification* notification = mNotificationRef->GetNotification();
-    if (!notification) {
-      return;
-    }
-
-    // Try to pass ownership back to the worker. If the dispatch succeeds we
-    // are guaranteed this runnable will run, and that it will run after queued
-    // event runnables, so event runnables will have a safe pointer to the
-    // Notification.
-    //
-    // If the dispatch fails, the worker isn't running anymore and the event
-    // runnables have already run. We can just let the standard NotificationRef
-    // release routine take over when ReleaseNotificationRunnable gets deleted.
-    class ReleaseNotificationRunnable final : public NotificationWorkerRunnable
-    {
-      UniquePtr<NotificationRef> mNotificationRef;
-    public:
-      explicit ReleaseNotificationRunnable(UniquePtr<NotificationRef> aRef)
-        : NotificationWorkerRunnable(aRef->GetNotification()->mWorkerPrivate)
-        , mNotificationRef(Move(aRef))
-      {}
-
-      void
-      WorkerRunInternal(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
-      {
-        UniquePtr<NotificationRef> ref;
-        mozilla::Swap(ref, mNotificationRef);
-        // Gets released at the end of the function.
-      }
-    };
-
-    nsRefPtr<ReleaseNotificationRunnable> r =
-      new ReleaseNotificationRunnable(Move(mNotificationRef));
-    notification = nullptr;
-
-    AutoJSAPI jsapi;
-    jsapi.Init();
-    r->Dispatch(jsapi.cx());
   }
 };
 

dom/notification/Notification.h

@@ -89,22 +89,12 @@ public:
  * Note that the Notification's JS wrapper does it's standard
  * AddRef()/Release() and is not affected by any of this.
  *
- * There is one case related to the WorkerNotificationObserver having to
- * dispatch WorkerRunnables to the worker thread which will use the
- * Notification object. We can end up in a situation where an event runnable is
- * dispatched to the worker, gets queued in the worker's event queue, but then,
- * the worker yields to the main thread. Here the main thread observer is
- * destroyed, which frees its NotificationRef. The NotificationRef dispatches
- * a ControlRunnable to the worker, which runs before the event runnable,
- * leading to the event runnable possibly not having a valid Notification
- * reference.
- * We solve this problem by having WorkerNotificationObserver's dtor
- * dispatching a standard WorkerRunnable to do the release (this guarantees the
- * ordering of the release is after the event runnables). All WorkerRunnables
- * that get dispatched successfully are guaranteed to run on the worker before
- * it shuts down. If that dispatch fails, the standard ControlRunnable based
- * shutdown is acceptable since the already dispatched event runnables have
- * already run or canceled (the worker is already past Running).
+ * Since the worker event queue can have runnables that will dispatch events on
+ * the Notification, the NotificationRef destructor will first try to release
+ * the Notification by dispatching a normal runnable to the worker so that it is
+ * queued after any event runnables. If that dispatch fails, it means the worker
+ * is no longer running and queued WorkerRunnables will be canceled, so we
+ * dispatch a control runnable instead.
  *
  */
 class Notification : public DOMEventTargetHelper