1948339 - Add NotificationObserver to keep handlers alive for backends without persistence support

Status: RESOLVED FIXED

(Core :: DOM: Notifications, enhancement)

Description

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

Currently Firefox closes push notifications when either the page or service worker goes away on Linux and macOS. We should keep them alive at least until Firefox process is closed, and for that there should be an entity that can outlive PNotification.

(Keeping handlers registered regardless of PNotification lifecycle would let notifications alive, but Alert Service alone can't and shouldn't handle service workers)

Comment 1

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

Attachment: Bug 1948339 - Part 1: Clean up unused header inclusion r=asuth

Mostly became redundant after IPC migration or even before that.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

Attachment: Bug 1948339 - Part 2: Add NotificationObserver r=asuth

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50816653be46
Part 1: Clean up unused header inclusion r=asuth

Comment 4

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/50816653be46

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53373d35978a
Part 2: Add NotificationObserver r=asuth

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/53373d35978a

Comment 7

[amarc]

Backed out for causing leaks @ browser/browser.toml

• Backout link
• Push with failures
• Failure Log

Comment 8

[Cristina Horotan [:chorotan]]

Backout merge to mozilla central: https://hg.mozilla.org/mozilla-central/rev/c84b2ed3f5b0bac47df6081736413822e08b8ff3

Comment 9

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

macOS specific because Windows and Linux counterparts listen to quit-application and clean things up. This should probably be done by nsAlertsService than on each system service.

Comment 10

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

(But technically it should be an existing problem)

Comment 11

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

So bug 1953906 can fix the issue in comment #7, but the real implication here is that the observer holds nsIPrincipal forever until the notification closes. That sounds bad, and maybe PrincipalInfo can be used to not do that? Thoughts?

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

We want to get rid of PrincipalInfo in favor of only ever holding nsIPrincipals. I hadn't appreciated that we might not be properly handling shutdown for notifications since that was more on the toolkit side from my perspective, but we should be handling shutdown.

Comment 13

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

Sure we should handle shutdown, but shouldn't we also prevent holding principals too long? Especially when holding principal also means holding windows and potentially make another ghost window issue.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

I also found that the current shutdown handler in widget/windows/ToastNotification is currently moot as the whole thing goes away before it ever listens quit-application.

(Seems I was wrong here)

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #13)

Sure we should handle shutdown, but shouldn't we also prevent holding principals too long? Especially when holding principal also means holding windows and potentially make another ghost window issue.

Can you elaborate on the ownership chain you are describing as it relates to comment 7, in particular if you're referring to a specific log and specific contents in it if you could provide a log viewer URL with the lines selected? If I look at the fields in ContentPrincipal I see how we can entrain atoms but I don't see how we can entrain windows. (That said there obviously are ways for a DOM binding that's not a GlobalTeardownObserver but depends on CC and could be part of a non-CC'ed graph if the observer service is also holding a strong ref.)

Comment 16

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

Per https://treeherder.mozilla.org/logviewer?job_id=498742864&repo=autoland&lineNumber=28189 it leaks dom/notification/test/browser/empty.html which the affected test uses, which IMO should not happen even if the observer leaks, as the observer should not hold the window in any way. It's unclear to me how that can happen either.

Comment 17

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

But perhaps that's just a red herring? 🤔

Comment 18

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2025-03-29)]

Per the private chat with :asuth, it seems the leak is because of the observer being XPConnect wrapper and thus references JSRuntime. I'd have never imagined that, thanks Andrew!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9f3e107f36c
Part 2: Add NotificationObserver r=asuth

Comment 20

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/f9f3e107f36c