Bug 916893 - Patch 2 - Deal with onclose. Some grammar fixes. r=wchen
☠☠ backed out by 531f100d2bd8 ☠ ☠
authorNikhil Marathe <nsm.nikhil@gmail.com>
Mon, 27 Apr 2015 11:54:48 -0700
changeset 281044 c3b07f0d1a6000e231395e3a2261f980271779e1
parent 281043 61c3f24cc908f0d46008dfa38939298841dfbee2
child 281045 7dc2448065a98863e8df27b21d9708921edf0057
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-beta@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerswchen
bugs916893
milestone41.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 916893 - Patch 2 - Deal with onclose. Some grammar fixes. r=wchen
dom/notification/Notification.cpp
dom/notification/Notification.h
--- a/dom/notification/Notification.cpp
+++ b/dom/notification/Notification.cpp
@@ -335,21 +335,24 @@ public:
 
   void
   WorkerRunInternal() override
   {
     mNotification->DispatchTrustedEvent(mEventName);
   }
 };
 
+// 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
   Forget()
   {
     mNotification = nullptr;
   }
 
@@ -359,22 +362,32 @@ public:
   {
     MOZ_ASSERT(mNotification);
     if (mNotification->mWorkerPrivate) {
       mNotification->mWorkerPrivate->AssertIsOnWorkerThread();
     } else {
       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);
         AutoSafeJSContext cx;
         if (!r->Dispatch(cx)) {
           MOZ_CRASH("Will leak worker thread Notification!");
         }
       } else {
@@ -385,16 +398,17 @@ public:
     }
   }
 
   // XXXnsm, is it worth having some sort of WeakPtr like wrapper instead of
   // a rawptr that the NotificationRef can invalidate?
   Notification*
   GetNotification()
   {
+    MOZ_ASSERT(Initialized());
     return mNotification;
   }
 };
 
 class NotificationTask : public nsRunnable
 {
 public:
   enum NotificationAction {
@@ -662,20 +676,25 @@ Notification::Constructor(const GlobalOb
                                                        aOptions);
   // Make a structured clone of the aOptions.mData object
   JS::Rooted<JS::Value> data(aGlobal.Context(), aOptions.mData);
   notification->InitFromJSVal(aGlobal.Context(), data, aRv);
   if (aRv.Failed()) {
     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"));
   }
 
   //XXXnsm The persistence service seems like a prime candidate of
   //PBackground.
   // On workers, persistence is done in the NotificationTask.
@@ -827,17 +846,16 @@ NS_IMPL_RELEASE_INHERITED(Notification, 
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(Notification)
 NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
 
 nsIPrincipal*
 Notification::GetPrincipal()
 {
   AssertIsOnMainThread();
-  MOZ_ASSERT_IF(mWorkerPrivate, mFeature);
   if (mWorkerPrivate) {
     return mWorkerPrivate->GetPrincipal();
   } else {
     nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryInterface(GetOwner());
     NS_ENSURE_TRUE(sop, nullptr);
     return sop->GetPrincipal();
   }
 }
@@ -1408,16 +1426,19 @@ Notification::WrapObject(JSContext* aCx,
   return mozilla::dom::NotificationBinding::Wrap(aCx, this, aGivenProto);
 }
 
 void
 Notification::Close()
 {
   AssertIsOnTargetThread();
   auto ref = MakeUnique<NotificationRef>(this);
+  if (!ref->Initialized()) {
+    return;
+  }
 
   nsCOMPtr<nsIRunnable> closeNotificationTask =
     new NotificationTask(Move(ref), NotificationTask::eClose);
   nsresult rv = NS_DispatchToMainThread(closeNotificationTask);
 
   if (NS_FAILED(rv)) {
     DispatchTrustedEvent(NS_LITERAL_STRING("error"));
     // If dispatch fails, NotificationTask will release the ref when it goes
@@ -1527,37 +1548,41 @@ void Notification::InitFromBase64(JSCont
   }
 
   auto container = new nsStructuredCloneContainer();
   aRv = container->InitFromBase64(aData, JS_STRUCTURED_CLONE_VERSION,
                                   aCx);
   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
 Notification::ReleaseObject()
 {
   AssertIsOnTargetThread();
   MOZ_ASSERT(mTaskCount > 0);
+  MOZ_ASSERT_IF(mWorkerPrivate, mFeature);
 
   --mTaskCount;
-  if (mWorkerPrivate && mFeature && mTaskCount == 0) {
+  if (mWorkerPrivate && mTaskCount == 0) {
     UnregisterFeature();
   }
   Release();
 }
 
 NotificationFeature::NotificationFeature(Notification* aNotification)
   : mNotification(aNotification)
 {
@@ -1607,31 +1632,25 @@ NotificationFeature::Notify(JSContext* a
 
   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;
 }
 
-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 =
-    mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(),
-                               mFeature.get());
-  if (!ok) {
-    MOZ_CRASH("AddFeature failed");
-  }
+  return mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(),
+                                    mFeature.get());
 }
 
 void
 Notification::UnregisterFeature()
 {
   MOZ_ASSERT(mWorkerPrivate);
   mWorkerPrivate->AssertIsOnWorkerThread();
   MOZ_ASSERT(mFeature);
--- a/dom/notification/Notification.h
+++ b/dom/notification/Notification.h
@@ -29,30 +29,30 @@ class Promise;
 
 namespace workers {
   class WorkerPrivate;
 } // namespace workers
 
 class Notification;
 class NotificationFeature final : public workers::WorkerFeature
 {
-  // Held alive by Notification itself before feature is added, and only
-  // released after feature is removed.
+  // Since the feature is strongly held by a Notification, it is ok to hold
+  // a raw pointer here.
   Notification* mNotification;
 
 public:
   explicit NotificationFeature(Notification* aNotification);
 
   bool
   Notify(JSContext* aCx, workers::Status aStatus) override;
 };
 
 
 /*
- * 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
  *   and a feature should be added to observe the worker lifetime. This main
  *   thread owner should ensure it properly releases the reference to the
  *   Notification, additionally removing the feature if necessary.
  *
  * To enforce the correct addref and release, along with managing the feature,
@@ -64,17 +64,24 @@ public:
  * Code should only access the underlying Notification object when it can
  * guarantee that it retains ownership of the NotificationRef while doing so.
  *
  * The one kink in this mechanism is that the worker feature may be Notify()ed
  * if the worker stops running script, even if the Notification's corresponding
  * 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
  *   associated observer. The NotificationRef itself is still owned by the
  *   observer and deleted by the UniquePtr, but it doesn't do anything since
  *   the underlying Notification is null.
  *
  * To unify code-paths, we use the same NotificationRef in the main
@@ -201,28 +208,34 @@ public:
 
   void InitFromBase64(JSContext* aCx, const nsAString& aData, ErrorResult& aRv);
 
   void AssertIsOnTargetThread() const
   {
     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;
 
   // The NotificationTask calls ShowInternal()/CloseInternal() on the
   // Notification. At this point the task has ownership of the Notification. It
   // 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,
                                                       ErrorResult& rv);
 
   bool DispatchClickEvent();
 protected:
   Notification(const nsAString& aID, const nsAString& aTitle, const nsAString& aBody,
@@ -306,22 +319,24 @@ private:
   nsresult PersistNotification();
   void UnpersistNotification();
 
   bool IsTargetThread() const
   {
     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;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_notification_h__