Bug 1187018 - Ensure feature is nulled out if it does not get added. r=khuey, a=ritu
authorNikhil Marathe <nsm.nikhil@gmail.com>
Fri, 24 Jul 2015 10:25:00 -0700
changeset 288726 129a17d4e2a40252bb86af441f0cc720c14bee0f
parent 288725 ecb0ccdde0978a5823aa54df23c3a077ae8ed967
child 288727 96e619dc79f07a6678a503a2ba2597d8ac0c2027
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)
reviewerskhuey, ritu
bugs1187018
milestone42.0a2
Bug 1187018 - Ensure feature is nulled out if it does not get added. r=khuey, a=ritu
dom/notification/Notification.cpp
--- a/dom/notification/Notification.cpp
+++ b/dom/notification/Notification.cpp
@@ -2083,62 +2083,97 @@ NotificationFeature::NotificationFeature
  *
  * We can freely access mNotification here because the feature supplied it and
  * the Notification owns the feature.
  */
 class CloseNotificationRunnable final
   : public WorkerMainThreadRunnable
 {
   Notification* mNotification;
+  bool mHadObserver;
 
   public:
   explicit CloseNotificationRunnable(Notification* aNotification)
     : WorkerMainThreadRunnable(aNotification->mWorkerPrivate)
     , mNotification(aNotification)
+    , mHadObserver(false)
   {}
 
   bool
   MainThreadRun() override
   {
     if (mNotification->mObserver) {
       // The Notify() take's responsibility of releasing the Notification.
       mNotification->mObserver->ForgetNotification();
       mNotification->mObserver = nullptr;
+      mHadObserver = true;
     }
     mNotification->CloseInternal();
     return true;
   }
+
+  bool
+  HadObserver()
+  {
+    return mHadObserver;
+  }
 };
 
 bool
 NotificationFeature::Notify(JSContext* aCx, Status aStatus)
 {
-  MOZ_ASSERT(aStatus >= Canceling);
+  if (aStatus >= Canceling) {
+    // CloseNotificationRunnable blocks the worker by pushing a sync event loop
+    // on the stack. Meanwhile, WorkerControlRunnables dispatched to the worker
+    // can still continue running. One of these is
+    // ReleaseNotificationControlRunnable that releases the notification,
+    // invalidating the notification and this feature. We hold this reference to
+    // keep the notification valid until we are done with it.
+    //
+    // An example of when the control runnable could get dispatched to the
+    // worker is if a Notification is created and the worker is immediately
+    // closed, but there is no permission to show it so that the main thread
+    // immediately drops the NotificationRef. In this case, this function blocks
+    // on the main thread, but the main thread dispatches the control runnable,
+    // invalidating mNotification.
+    nsRefPtr<Notification> kungFuDeathGrip = mNotification;
 
-  // Dispatched to main thread, blocks on closing the Notification.
-  nsRefPtr<CloseNotificationRunnable> r =
-    new CloseNotificationRunnable(mNotification);
-  r->Dispatch(aCx);
+    // Dispatched to main thread, blocks on closing the Notification.
+    nsRefPtr<CloseNotificationRunnable> r =
+      new CloseNotificationRunnable(mNotification);
+    r->Dispatch(aCx);
 
-  mNotification->ReleaseObject();
-  // From this point we cannot touch properties of this feature because
-  // ReleaseObject() may have led to the notification going away and the
-  // notification owns this feature!
+    // Only call ReleaseObject() to match the observer's NotificationRef
+    // ownership (since CloseNotificationRunnable asked the observer to drop the
+    // reference to the notification).
+    if (r->HadObserver()) {
+      mNotification->ReleaseObject();
+    }
+
+    // From this point we cannot touch properties of this feature because
+    // ReleaseObject() may have led to the notification going away and the
+    // notification owns this feature!
+  }
   return true;
 }
 
 bool
 Notification::RegisterFeature()
 {
   MOZ_ASSERT(mWorkerPrivate);
   mWorkerPrivate->AssertIsOnWorkerThread();
   MOZ_ASSERT(!mFeature);
   mFeature = MakeUnique<NotificationFeature>(this);
-  return mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(),
-                                    mFeature.get());
+  bool added = mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(),
+                                          mFeature.get());
+  if (!added) {
+    mFeature = nullptr;
+  }
+
+  return added;
 }
 
 void
 Notification::UnregisterFeature()
 {
   MOZ_ASSERT(mWorkerPrivate);
   mWorkerPrivate->AssertIsOnWorkerThread();
   MOZ_ASSERT(mFeature);
@@ -2300,24 +2335,24 @@ Notification::CreateAndShow(nsIGlobalObj
   JSContext* cx = jsapi.cx();
 
   nsRefPtr<Notification> notification = CreateInternal(aGlobal, EmptyString(),
                                                        aTitle, aOptions);
 
   // Make a structured clone of the aOptions.mData object
   JS::Rooted<JS::Value> data(cx, aOptions.mData);
   notification->InitFromJSVal(cx, data, aRv);
-  if (aRv.Failed()) {
+  if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
 
   notification->SetScope(aScope);
 
   auto ref = MakeUnique<NotificationRef>(notification);
-  if (!ref->Initialized()) {
+  if (NS_WARN_IF(!ref->Initialized())) {
     aRv.Throw(NS_ERROR_DOM_ABORT_ERR);
     return nullptr;
   }
 
   // Queue a task to show the notification.
   nsCOMPtr<nsIRunnable> showNotificationTask =
     new NotificationTask(Move(ref), NotificationTask::eShow);
   nsresult rv = NS_DispatchToMainThread(showNotificationTask);