1847096 - Clicking notification without an awake service worker always opens a new window with the root path of the origin (on Windows) or does nothing (on Android)

Status: RESOLVED FIXED

(Core :: DOM: Notifications, defect, P2)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

What we currently do is just opening the origin; e.g. https://app.slack.com for all Slack notifications. We do not trigger notificationclick event on service worker, but we should.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

So, the code path differs quite from bug 1874511 because we can't depend on Activated event in this case.

1. Windows invokes the Firefox notification server as described in launchArg generated from ToastNotificationHandler::GetLaunchArgument. This includes host/port pair via launchUrl generated by ToastNotificationHandler::ActionArgsJSONString, which then is served as action parameter.
2. The server then again invokes Firefox with --notification-windowsAction.
3. BrowserContentHandler calls handleWindowsTag, which fails with tagWasHandled: false as ToastNotification::VerifyTagPresentOrFallback fails to find any match with mActiveHandlers, which is currently only initiated by ToastNotification::ShowAlert.
4. The failure fallback either opens a tab or a new window with notificationData.launchUrl depending on the current browser state.

(These steps actually happen for both cases per the comment, but is ignored when tag is handled. Or not, see bug 1880863.)

I think we need to reload notification handlers from the notification DB, as I don't think that's happening right now. Each notification in DB has alertName for unique match.

Comment 2

[David]

Would you please remove the "when the browser is closed" from the title? Since there seem to be no conditions at all where it Does run the service worker.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

"Notification click" is the condition, but sure I can clarify.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Actually this is a defect, so rephrasing the title again to match that.

Comment 5

[David]

That new title places this even farther from the issue I reported which was referred here in bug 1874511, comment 8

I Never click notifications when the browser is closed, it's always open, and clicking them when it's open Always goes to the front page (the main domain) instead of the correct address (as mentioned in comment 0 here) if it goes anywhere at all. Not going anywhere is covered now by bug 1874511.

Including 'when closed' in the title narrows the scope way smaller than the issue is. It's a major user facing problem which significantly reduces the utility of the browser.
Unless I'm the only one seeing this? I have a few extensions but it's a relatively new profile.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

If it always fails then we need more info, can you try:

1. Open https://simple-push-demo.vercel.app/
2. Enable push
3. Click "Send a push message" immediately when notification shows

Clicking it should open https://web.dev/articles/push-notifications-overview .

Comment 7

[David]

Very interesting. I restarted the browser for an update a few days ago and when it came back this issue didn't happen - it started going to the correct page for every notification that opened at all, including that test page. I'll keep an eye on it, it seems like it's either not the 100% repro I was seeing before or it's been fixed entirely (while open. I still haven't tested the closed case.)

I'll keep the needsinfo on while I monitor the behavior.

Comment 8

[David]

Sorry I didn't get back to you about this. I have seen it happen (when the browser is open) just a few times since then, but it's also difficult to test now because Bug 1874511 is such a common repro.

So it still happens, but rarely anymore.

Comment 9

[Jens Stutte [:jstutte]]

Do we have enough information to set the severity here? Thank you!

Comment 10

[Nick Alexander :nalexander [he/him]]

krosylight: my WNS PoC ends up invoking notifyPushWithData at about the same place where native notifications are handled.

It might be possible to arrange for some Web Push specific data to round-trip through the toast notification system to support this use case, if you'd like it to. The issue might be identifying when a toast notification "originates" from a Web Push -- but I think that's more or less always here. Food for thought.

Comment 11

[Jens Stutte [:jstutte]]

(for comment 10)

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

It should be possible to pass a service worker identifier to each notification so that the command would include it as a parameter. I'm not quite sure we should do that, because:

• I'm not sure it's exactly safe, as some malware could scan the notification center and craft a command with the identifier to mock a user click, which then would run JS with malware crafted arguments. But maybe we can't do much for such an infected machine, as there would be hundreds more evil things to do for a malware?
• Do we know the same can be done for Linux and macOS, i.e. can we store the same amount of information into libnotify and macOS notification system?
  • Can you share how you did it for Windows? Bug 1803416 only has the proxy binary part, and per my understanding we hash the information here to fit the system limitation. My plan was to use this hashed tag everywhere (in other backends and notification store), but open for better ideas!

Please feel free to correct me if I got something wrong 👍🏻

Comment 13

[David]

Just a quick update that since Bug 1874511 was fixed, this one has taken over as the main thing that happens when a notification is clicked. (Unless the browser was just recently restarted). It's no longer in any way a rare occurrence.

Comment 15

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1847096 - Part 1: Split most of NotificationParent::Show into NotificationManager::Create

Ultimately showNotification() should be able to directly call NotificationManager without having to go through PNotification and the full blown NotificationObserver.

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1847096 - Part 2: Split click listener of NotificationObserver into helper functions r=asuth

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1847096 - Part 3: Implement NotificationHandler::RespondOnClick r=asuth

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1847096 - Part 4: Split handleNotification from nsDefaultCommandLineHandler.handle r=gijs

Requested in D236586. This is just a pure code style change for easier review, migration will happen in the next patch.

Comment 19

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1847096 - Part 5: Use NotificationHandler in BrowserContentHandler r=gijs

Comment 20

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1847096 - reuse?

Comment 21

[Kagami Rosylight [:saschanaz] (they/them)]

Answering Gijs' question in https://phabricator.services.mozilla.com/D250061#inline-1394374:

./mach run --notification-windowsTag=foo --notification-windowsAction='{"origin": "https://example.org"}' is a way to simulate the notification click command. With the current patch stack,

1. It opens a startup window
2. And then opens a new browser window for example.org
3. But the window at step 1 remains, being totally blank with no UI at all

What we can do:

1. Somehow figure it out how to reuse the startup window
2. Just start a full browser and open a new tab in the existing window

I'm leaning to 2 because:

1. It's easier
2. Aligns with existing commandline handler behavior - Running firefox https://example.com when there's only Library window also kickstarts a new browser window and then opens the webpage, restoring the previous session if the option is enabled

What do you think? Depending on the answer I'll also need to adjust the patches in bug 1218080.

(Edit: Actually the current bug 1218080 also effectively does 2 with optional session restore. Cool, I'll double check if this is the case for this bug too.)

Comment 22

[Kagami Rosylight [:saschanaz] (they/them)]

Ok, no need to even ask, I'll do that path. The current behavior sounds just wrong.

Comment 23

[Kagami Rosylight [:saschanaz] (they/them)]

What I don't understand is, why bug 1218080 can do the session restore while this bug cannot? Both depend on the same function OpenNewWindow added in bug 1218080 🤔

Comment 24

[Phabricator Automation]

Comment on attachment 9488745 [details]
Bug 1847096 - Part 4: Split handleNotification from nsDefaultCommandLineHandler.handle r=gijs

Revision D250060 was moved to bug 1974234. Setting attachment 9488745 [details] to obsolete.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ce19c63f0ed3
https://hg.mozilla.org/integration/autoland/rev/e10c385f31d2
Part 2: Split click listener of NotificationObserver into helper functions r=asuth
https://github.com/mozilla-firefox/firefox/commit/7263e626165b
https://hg.mozilla.org/integration/autoland/rev/79bf8198c3d2
Part 3: Implement NotificationHandler::RespondOnClick r=asuth

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/fe84f0eba000
https://hg.mozilla.org/integration/autoland/rev/195338ae12bf
Revert "Bug 1847096 - Part 3: Implement NotificationHandler::RespondOnClick r=asuth" for causing build bustages @NotificationParent.cpp and @NotificationHandler.h.

Comment 27

[agoloman]

Backed out for causing build bustages @NotificationParent.cpp and @NotificationHandler.h.

• Backout link
• Push with failures
• Failure Log @NotificationParent.cpp
• Failure Log @NotificationHandler.h.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8cab8eac03c1
https://hg.mozilla.org/integration/autoland/rev/d174d37b8b53
Part 2: Split click listener of NotificationObserver into helper functions r=asuth
https://github.com/mozilla-firefox/firefox/commit/e97d16512fe4
https://hg.mozilla.org/integration/autoland/rev/b8f996b1dfb3
Part 3: Implement NotificationHandler::RespondOnClick r=asuth
https://github.com/mozilla-firefox/firefox/commit/b7995e3d0739
https://hg.mozilla.org/integration/autoland/rev/19f615d8b22e
apply code formatting via Lando

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7e1408c7cc9a
https://hg.mozilla.org/integration/autoland/rev/3e6ba9b16281
Revert "Bug 1847096: apply code formatting via Lando" for causing multiple wpt failures Notification related.

Comment 30

[Cristian Tuns]

Backed out for causing wpt failures.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-ERROR | /_mozilla/notifications/simulate-click-handler.sub.https.window.html | 1 duplicate test name: "Fire notificationclick via NotificationHandler"

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/18b666f73101
https://hg.mozilla.org/integration/autoland/rev/a88fbd78190d
Part 2: Split click listener of NotificationObserver into helper functions r=asuth
https://github.com/mozilla-firefox/firefox/commit/bfc4594d3f8d
https://hg.mozilla.org/integration/autoland/rev/0d634bf7b3a8
Part 3: Implement NotificationHandler::RespondOnClick r=asuth
https://github.com/mozilla-firefox/firefox/commit/783629d82b8b
https://hg.mozilla.org/integration/autoland/rev/dcdce87f5be9
apply code formatting via Lando

Comment 32

[[e-mail]]

https://hg.mozilla.org/mozilla-central/rev/a88fbd78190d
https://hg.mozilla.org/mozilla-central/rev/0d634bf7b3a8
https://hg.mozilla.org/mozilla-central/rev/dcdce87f5be9

Comment 33

[Phabricator Automation]

Comment on attachment 9488746 [details]
Bug 1847096 - Part 5: Use NotificationHandler in BrowserContentHandler r=gijs

Revision D250061 was moved to bug 1976474. Setting attachment 9488746 [details] to obsolete.

Comment 34

[Nick Alexander :nalexander [he/him]]

Clearing NI due to :krosylight's good work!