Backed out 2 changesets (bug 1879112, bug 1879934) for causing assertion failures @ widget/windows/ToastNotification.cpp

Backed out changeset 2b3b6edc0cff (bug 1879112)
Backed out changeset 3dd0aa903d7f (bug 1879934)

dom/notification/Notification.cpp

@@ -714,12 +714,7 @@ Notification::Notification(nsIGlobalObject* aGlobal, const nsAString& aID,
   }
 }
 
-nsresult Notification::MaybeObserveWindowFrozenOrDestroyed() {
-  // NOTE: Non-persistent notifications can also be opened from workers, but we
-  // don't care and nobody else cares. And it's not clear whether we even should
-  // do this for window at all, see
-  // https://github.com/whatwg/notifications/issues/204.
-  // TODO: Somehow extend GlobalTeardownObserver to deal with FROZEN_TOPIC?
+nsresult Notification::Init() {
   if (!mWorkerPrivate) {
     nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
     NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE);
@@ -781,10 +776,6 @@ already_AddRefed<Notification> Notification::Constructor(
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
-  if (NS_WARN_IF(
-          NS_FAILED(notification->MaybeObserveWindowFrozenOrDestroyed()))) {
-    return nullptr;
-  }
 
   // This is be ok since we are on the worker thread where this function will
   // run to completion before the Notification has a chance to go away.
@@ -924,6 +915,8 @@ already_AddRefed<Notification> Notification::CreateInternal(
       aGlobal, id, aTitle, aOptions.mBody, aOptions.mDir, aOptions.mLang,
       aOptions.mTag, aOptions.mIcon, aOptions.mRequireInteraction, silent,
       std::move(vibrate), aOptions.mMozbehavior);
+  rv = notification->Init();
+  NS_ENSURE_SUCCESS(rv, nullptr);
   return notification.forget();
 }
 
@@ -1316,8 +1309,6 @@ bool Notification::IsInPrivateBrowsing() {
   return false;
 }
 
-// Step 4 of
-// https://notifications.spec.whatwg.org/#dom-notification-notification
 void Notification::ShowInternal() {
   AssertIsOnMainThread();
   MOZ_ASSERT(mTempRef,
@@ -1332,15 +1323,13 @@ void Notification::ShowInternal() {
   std::swap(ownership, mTempRef);
   MOZ_ASSERT(ownership->GetNotification() == this);
 
+  nsresult rv = PersistNotification();
+  if (NS_FAILED(rv)) {
+    NS_WARNING("Could not persist Notification");
+  }
+
   nsCOMPtr<nsIAlertsService> alertService = components::Alerts::Service();
 
-  // Step 4.1: If the result of getting the notifications permission state is
-  // not "granted", then queue a task to fire an event named error on this, and
-  // abort these steps.
-  //
-  // XXX: But this function is also triggered by
-  // Notification::ShowPersistentNotification which already does its own
-  // permission check. Can we split this?
   ErrorResult result;
   NotificationPermission permission = NotificationPermission::Denied;
   if (mWorkerPrivate) {
@@ -1361,28 +1350,13 @@ void Notification::ShowInternal() {
     } else {
       DispatchTrustedEvent(u"error"_ns);
     }
-    mIsClosed = true;
     return;
   }
 
-  // Preparing for Step 4.2 the fetch steps. The actual work happens in
-  // nsIAlertNotification::LoadImage
   nsAutoString iconUrl;
   nsAutoString soundUrl;
   ResolveIconAndSoundURL(iconUrl, soundUrl);
 
-  // Step 4.3 the show steps, which are almost all about processing `tag` and
-  // then displaying the notification. Both are handled by
-  // nsIAlertsService::ShowAlert/PersistentNotification. The below is all about
-  // constructing the observer (for show and close events) right and ultimately
-  // call the alerts service function.
-
-  // XXX: Non-persistent notifications probably don't need this
-  nsresult rv = PersistNotification();
-  if (NS_FAILED(rv)) {
-    NS_WARNING("Could not persist Notification");
-  }
-
   bool isPersistent = false;
   nsCOMPtr<nsIObserver> observer;
   if (mScope.IsEmpty()) {

dom/notification/Notification.h

@@ -245,8 +245,7 @@ class Notification : public DOMEventTargetHelper,
       nsIGlobalObject* aGlobal, const nsAString& aID, const nsAString& aTitle,
       const NotificationOptions& aOptions, ErrorResult& aRv);
 
-  // Triggers CloseInternal for non-persistent notifications if window goes away
-  nsresult MaybeObserveWindowFrozenOrDestroyed();
+  nsresult Init();
   bool IsInPrivateBrowsing();
   void ShowInternal();
   void CloseInternal(bool aContextClosed = false);

testing/web-platform/meta/notifications/shownotification-window.https.html.ini

@@ -1 +0,0 @@
-prefs: [notification.prompt.testing:true,marionette.setpermission.enabled:true]

testing/web-platform/tests/notifications/global-teardown-crash.html

@@ -1,16 +0,0 @@
-<!DOCTYPE html>
-<html class="test-wait">
-<meta charset="utf-8">
-<iframe id="iframe" srcdoc="
-  <script>
-  window.onload = () => {
-    new Notification('foo');
-    if (window.parent.ran) {
-      window.parent.document.documentElement.classList.remove('test-wait');
-    } else {
-      window.parent.ran = true;
-      location.reload();
-    }
-  }
-  </script>
-"></iframe>

testing/web-platform/tests/notifications/resources/helpers.js

@@ -12,24 +12,9 @@ async function getActiveServiceWorker(script) {
   return reg;
 }
 
+
 async function closeAllNotifications() {
   for (const n of await registration.getNotifications()) {
     n.close();
   }
 }
-
-async function trySettingPermission(perm) {
-  try {
-    await test_driver.set_permission({ name: "notifications" }, perm);
-  } catch {
-    // Not all implementations support this yet, but the permission may already be set to be able to continue
-  }
-
-  // Using Notification.permission instead of permissions.query() as
-  // some implementation without set_permission support overrides
-  // Notification.permission.
-  const permission = Notification.permission === "default" ? "prompt" : Notification.permission;
-  if (permission !== perm) {
-    throw new Error(`Should have the permission ${perm} to continue`);
-  }
-}

testing/web-platform/tests/notifications/resources/shownotification-window-iframe.html

@@ -1,8 +0,0 @@
-<!DOCTYPE html>
-<meta charset="utf-8">
-<script>
-  async function showNotification() {
-    const registration = await navigator.serviceWorker.ready;
-    await registration.showNotification('foo');
-  }
-</script>

testing/web-platform/tests/notifications/shownotification-window.https.html

@@ -1,37 +0,0 @@
-<!DOCTYPE html>
-<meta charset="utf-8">
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<script src="/resources/testdriver.js"></script>
-<script src="/resources/testdriver-vendor.js"></script>
-<script src="resources/helpers.js"></script>
-<iframe id="iframe" src="resources/shownotification-window-iframe.html"></iframe>
-<script>
-/** @type {ServiceWorkerRegistration} */
-let registration;
-
-promise_setup(async (t) => {
-  await trySettingPermission("granted");
-  registration = await getActiveServiceWorker("noop-sw.js");
-  await closeAllNotifications();
-});
-
-promise_test(async (t) => {
-  t.add_cleanup(closeAllNotifications);
-
-  if (iframe.contentDocument.readyState !== "complete") {
-    await new Promise(resolve => iframe.onload = resolve);
-  }
-
-  await iframe.contentWindow.showNotification();
-  let notifications = await registration.getNotifications();
-  assert_equals(notifications.length, 1, "Should persist the notification");
-
-  iframe.contentWindow.location.reload();
-  // Wait for some time for potential notification close requests to be sent
-  await new Promise(resolve => iframe.onload = resolve);
-  notifications = await registration.getNotifications();
-  assert_equals(notifications.length, 1, "Should keep the notification");
-}, 'Refreshing window does not clear persistent notifications');
-</script>
-</body>

testing/web-platform/tests/notifications/shownotification-without-permission.https.window.js

@@ -8,14 +8,20 @@
 let registration;
 
 promise_setup(async () => {
-  await trySettingPermission("prompt");
   registration = await getActiveServiceWorker("noop-sw.js");
-  await closeAllNotifications();
 });
 
-promise_test(async t => {
+promise_test(async (t) => {
   t.add_cleanup(closeAllNotifications);
 
+  try {
+    await test_driver.set_permission({ name: "notifications" }, "prompt");
+  } catch {
+    // Not all implementations support this yet, but it may already be "prompt" to be able to continue
+  }
+
+  assert_equals(Notification.permission, "default", "Should have the default permission to continue");
+
   await promise_rejects_js(t, TypeError, registration.showNotification(""), "Should throw TypeError");
   const notifications = await registration.getNotifications();
   assert_equals(notifications.length, 0, "Should return zero notification");

widget/windows/ToastNotification.cpp

@@ -776,8 +776,6 @@ ToastNotification::CloseAlert(const nsAString& aAlertName,
                               bool aContextClosed) {
   RefPtr<ToastNotificationHandler> handler;
   if (NS_WARN_IF(!mActiveHandlers.Get(aAlertName, getter_AddRefs(handler)))) {
-    MOZ_ASSERT_UNREACHABLE(
-        "Closing a notification that is not being handled by nsIAlertsService");
     return NS_OK;
   }