Bug 916893 - Patch 2 - Deal with onclose. Some grammar fixes. r=wchen

dom/notification/Notification.cpp

@@ -340,11 +340,14 @@ public:
   }
 };
 
+// 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;
 
   // Only useful for workers.
   void
@@ -364,12 +367,22 @@ public:
       AssertIsOnMainThread();
     }
 
-    mNotification->AddRefObject();
+    mInited = mNotification->AddRefObject();
+  }
+
+  // This is only required because Gecko runs script in a worker's onclose
+  // handler (non-standard, Bug 790919) where calls to AddFeature() will fail.
+  // Due to non-standardness and added complications if we decide to support
+  // this, attempts to create a Notification in onclose just throw exceptions.
+  bool
+  Initialized()
+  {
+    return mInited;
   }
 
   ~NotificationRef()
   {
-    if (mNotification) {
+    if (Initialized() && mNotification) {
       if (mNotification->mWorkerPrivate && NS_IsMainThread()) {
         nsRefPtr<ReleaseNotificationControlRunnable> r =
           new ReleaseNotificationControlRunnable(mNotification);
@@ -390,6 +403,7 @@ public:
   Notification*
   GetNotification()
   {
+    MOZ_ASSERT(Initialized());
     return mNotification;
   }
 };
@@ -667,10 +681,15 @@ Notification::Constructor(const GlobalObject& aGlobal,
     return nullptr;
   }
 
-  auto n = MakeUnique<NotificationRef>(notification);
+  auto ref = MakeUnique<NotificationRef>(notification);
+  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(n), NotificationTask::eShow);
+    new NotificationTask(Move(ref), NotificationTask::eShow);
   nsresult rv = NS_DispatchToMainThread(showNotificationTask);
   if (NS_FAILED(rv)) {
     notification->DispatchTrustedEvent(NS_LITERAL_STRING("error"));
@@ -832,7 +851,6 @@ nsIPrincipal*
 Notification::GetPrincipal()
 {
   AssertIsOnMainThread();
-  MOZ_ASSERT_IF(mWorkerPrivate, mFeature);
   if (mWorkerPrivate) {
     return mWorkerPrivate->GetPrincipal();
   } else {
@@ -1413,6 +1431,9 @@ Notification::Close()
 {
   AssertIsOnTargetThread();
   auto ref = MakeUnique<NotificationRef>(this);
+  if (!ref->Initialized()) {
+    return;
+  }
 
   nsCOMPtr<nsIRunnable> closeNotificationTask =
     new NotificationTask(Move(ref), NotificationTask::eClose);
@@ -1532,17 +1553,20 @@ void Notification::InitFromBase64(JSContext* aCx, const nsAString& aData,
   mDataObjectContainer = container;
 }
 
-void
+bool
 Notification::AddRefObject()
 {
   AssertIsOnTargetThread();
-  AddRef();
+  MOZ_ASSERT_IF(mWorkerPrivate && !mFeature, mTaskCount == 0);
-  MOZ_ASSERT_IF(mWorkerPrivate&& !mFeature, mTaskCount == 0);
   MOZ_ASSERT_IF(mWorkerPrivate && mFeature, mTaskCount > 0);
   if (mWorkerPrivate && !mFeature) {
-    RegisterFeature();
+    if (!RegisterFeature()) {
+      return false;
+    }
   }
+  AddRef();
   ++mTaskCount;
+  return true;
 }
 
 void
@@ -1550,9 +1574,10 @@ Notification::ReleaseObject()
 {
   AssertIsOnTargetThread();
   MOZ_ASSERT(mTaskCount > 0);
+  MOZ_ASSERT_IF(mWorkerPrivate, mFeature);
 
   --mTaskCount;
-  if (mWorkerPrivate && mFeature && mTaskCount == 0) {
+  if (mWorkerPrivate && mTaskCount == 0) {
     UnregisterFeature();
   }
   Release();
@@ -1612,21 +1637,15 @@ NotificationFeature::Notify(JSContext* aCx, Status aStatus)
   return true;
 }
 
-void
+bool
 Notification::RegisterFeature()
 {
   MOZ_ASSERT(mWorkerPrivate);
   mWorkerPrivate->AssertIsOnWorkerThread();
   MOZ_ASSERT(!mFeature);
-  // Only calls to show() and close() should lead to AddFeature(), both of
-  // which come from JS. So we can assert that AddFeature() should succeed.
   mFeature = MakeUnique<NotificationFeature>(this);
-  bool ok =
+  return mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(),
-    mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(),
+                                    mFeature.get());
-                               mFeature.get());
-  if (!ok) {
-    MOZ_CRASH("AddFeature failed");
-  }
 }
 
 void

dom/notification/Notification.h

@@ -34,8 +34,8 @@ namespace workers {
 class Notification;
 class NotificationFeature final : public workers::WorkerFeature
 {
-  // Held alive by Notification itself before feature is added, and only
+  // Since the feature is strongly held by a Notification, it is ok to hold
-  // released after feature is removed.
+  // a raw pointer here.
   Notification* mNotification;
 
 public:
@@ -47,7 +47,7 @@ public:
 
 
 /*
- * Notifications on workers introduce some liveness issues. The property we
+ * Notifications on workers introduce some lifetime issues. The property we
  * are trying to satisfy is:
  *   Whenever a task is dispatched to the main thread to operate on
  *   a Notification, the Notification should be addrefed on the worker thread
@@ -69,7 +69,14 @@ public:
  * UI is still visible to the user. We handle this case with the following
  * steps:
  *   a) Close the notification. This is done by blocking the worker on the main
- *   thread.
+ *   thread. This ensures that there are no main thread holders when the worker
+ *   resumes. This also deals with the case where Notify() runs on the worker
+ *   before the observer has been created on the main thread. Even in such
+ *   a situation, the CloseNotificationRunnable() will only run after the
+ *   Show task that was previously queued. Since the show task is only queued
+ *   once when the Notification is created, we can be sure that no new tasks
+ *   will follow the Notify().
+ *
  *   b) Ask the observer to let go of its NotificationRef's underlying
  *   Notification without proper cleanup since the feature will handle the
  *   release. This is only OK because every notification has only one
@@ -206,7 +213,10 @@ public:
     MOZ_ASSERT(IsTargetThread());
   }
 
+  // Initialized on the worker thread, never unset, and always used in
+  // a read-only capacity. Used on any thread.
   workers::WorkerPrivate* mWorkerPrivate;
+
   // Main thread only.
   WorkerNotificationObserver* mObserver;
 
@@ -215,9 +225,12 @@ public:
   // passes this on to the Notification itself via mTempRef so that
   // ShowInternal()/CloseInternal() may pass it along appropriately (or release
   // it).
+  //
+  // Main thread only.
   UniquePtr<NotificationRef> mTempRef;
 
-  void AddRefObject();
+  // Returns true if addref succeeded.
+  bool AddRefObject();
   void ReleaseObject();
 
   static NotificationPermission GetPermissionInternal(nsIPrincipal* aPrincipal,
@@ -311,12 +324,14 @@ private:
     return NS_IsMainThread() == !mWorkerPrivate;
   }
 
-  void RegisterFeature();
+  bool RegisterFeature();
   void UnregisterFeature();
 
   nsresult ResolveIconAndSoundURL(nsString&, nsString&);
 
+  // Only used for Notifications on Workers, worker thread only.
   UniquePtr<NotificationFeature> mFeature;
+  // Target thread only.
   uint32_t mTaskCount;
 };