Bug 1196079 - Always try to release Notification via normal WorkerRunnable first. r=wchen, a=ritu
authorNikhil Marathe <nsm.nikhil@gmail.com>
Mon, 24 Aug 2015 15:40:57 -0700
changeset 289070 e58ce20624cd6c75a00ccb69f39edf58d731a07d
parent 289069 cff5853462a02afc36d1249dd76a7b689af6a854
child 289071 0018e75dd248bc040607b149968ce179dcdc4d96
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerswchen, ritu
bugs1196079
milestone42.0a2
Bug 1196079 - Always try to release Notification via normal WorkerRunnable first. r=wchen, a=ritu
dom/notification/Notification.cpp
dom/notification/Notification.h
--- a/dom/notification/Notification.cpp
+++ b/dom/notification/Notification.cpp
@@ -404,16 +404,32 @@ public:
 
   void
   WorkerRunInternal(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
   {
     mNotification->DispatchTrustedEvent(mEventName);
   }
 };
 
+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 {
   friend class WorkerNotificationObserver;
 
 private:
   Notification* mNotification;
   bool mInited;
@@ -447,28 +463,41 @@ public:
   Initialized()
   {
     return mInited;
   }
 
   ~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;
     }
   }
 
   // XXXnsm, is it worth having some sort of WeakPtr like wrapper instead of
   // a rawptr that the NotificationRef can invalidate?
   Notification*
   GetNotification()
   {
@@ -968,54 +997,16 @@ public:
   }
 
 protected:
   virtual ~WorkerNotificationObserver()
   {
     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());
   }
 };
 
 NS_IMPL_ISUPPORTS_INHERITED0(WorkerNotificationObserver, NotificationObserver)
 
 class ServiceWorkerNotificationObserver final : public nsIObserver
 {
 public:
--- a/dom/notification/Notification.h
+++ b/dom/notification/Notification.h
@@ -84,32 +84,22 @@ public:
  *   the underlying Notification is null.
  *
  * To unify code-paths, we use the same NotificationRef in the main
  * thread implementation too.
  *
  * 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
 {
   friend class CloseNotificationRunnable;
   friend class NotificationTask;
   friend class NotificationPermissionRequest;
   friend class NotificationObserver;