1863798 - Clicking on new mail notification on Windows doesn't bring the application to the foreground

Status: VERIFIED FIXED

(Thunderbird :: General, defect, P2)

Description

[doelli]

+++ This bug was initially created as a clone of Bug #1858603 +++

Steps to reproduce:

1. Make sure you have both notifications and system notifications turned on.
2. Wait for a new mail.
3. Notification pops up in the Notifications section in Windows 11.
4. Click on the notification itself.

Actual results:

Nothing happens.

Expected results:

Clicking on notification should open up Thunderbird and the mail that we've been notified about.

TB 120 Beta: After setting alerts.useSystemBackend to true (see bug 1862407), I see this behaviour:

1. It takes about 10 seconds for TB to become active.
2. A minimised window is restored, but a non-minimised window in the background is not brought to the foreground.

Comment 1

[betterbird.project+11]

We looked at the two issues a bit. The delayed activation is quite a mystery, we filed this upstream ticket for it.

The code in ToastNotificationHandler::OnActivate() here
https://searchfox.org/mozilla-central/rev/02841791400cf7cf5760c0cfaf31f5d772624253/widget/windows/ToastNotificationHandler.cpp#873
doesn't work for Thunderbird. It can easily be fixed with this patch:
https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1863798-toast-notification-foreground-mail-client-m-c.patch
since GetMostRecentBrowserWindow() also works for Thunderbird. However, observed behaviour is that the window is still not brought to the foreground, even minimised windows are not reliably restored, sometimes only their taskbar button flashes.

That's particularly surprising since this code
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/nsMessengerWinIntegration.cpp#79,166
works reliably.

There is a hint in this article (quote):
SetForegroundWindow() is the correct way to change the foreground window, but the thread that calls SetForegroundWindow() has to meet certain criteria for it to "work". If it does not meet the criteria, the window's taskbar button is flashed instead. This is by design. This is to protect the user from applications stealing focus and you should respect that.

Looks like the Toast activation used a different thread than the system tray activation in nsMessengerWinIntegration.cpp.

Strangely the activation calls this:
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/MailNotificationManager.jsm#99
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mail/modules/MailUtils.jsm#359
And .showWindow() calls this:
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/nsMessengerWinIntegration.cpp#230
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/nsMessengerWinIntegration.cpp#79
which is the code that works for system tray enabling.

Makoto san, do you have any insights?

Comment 2

[betterbird.project+11]

Further debugging shows that the SetForegroundWindow() calls here
https://searchfox.org/mozilla-central/rev/02841791400cf7cf5760c0cfaf31f5d772624253/widget/windows/ToastNotificationHandler.cpp#879
and here
https://searchfox.org/comm-central/rev/14314db9cf7d340d3249b69451427b9f0a13930d/mailnews/base/src/nsMessengerWinIntegration.cpp#79
fail; they apparently don't fulfil the requirements set out here:
https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-setforegroundwindow
since the Thunderbird process making the call didn't receive the last input.

Comment 3

[betterbird.project+11]

Nick, you fixed bug 1795974. Can you help with this bug here? As per comment #0 there are two problems:

1. It takes about 10 seconds for before ToastNotificationHandler::OnActivate() gets called
2. SetForegroundWindow() here and here doesn't have the permissions to bring the Mozilla app window to the foreground, likely because the process handling the click didn't grant that privilege.

Questions:

1. Any insights re. the activation delay?
2. Does the SetForegroundWindow() at ToastNotificationHandler.cpp#879 work in FF or does it fail silently?
3. In ToastNotification.cpp#609 (from bug 1795974) you allude to (quote) "Send our window's PID to the notification server", but is that actually done? The PID is a local variable, there is no call to AllowSetForegroundWindow() we can see anywhere. Besides, ToastNotification::SignalComNotificationHandled() is only called as part of ToastNotification::HandleWindowsTag().

Comment 4

[betterbird.project+11]

For the record, on codeproject.com we got given this advice:

You can pass the PID to the WinToastHandler constructor before you call showToast(). It is up to you to add the PID member to WinToastHandler class.

std::shared_ptr<IWinToastHandler> handler(new WinToastHandler(this));
if (WinToast::instance()->showToast(templ, handler) == -1L)
{...}

If we interpret this correctly, the Mozilla app PID should be retrieved here and added to the Toast handler:
https://searchfox.org/mozilla-central/rev/27ddf0064aa821af8238a99621df79fb1b301c7b/toolkit/mozapps/defaultagent/Notification.cpp#511
Then upon activation in toastActivated() in the handler a call to AllowSetForegroundWindow() should be made with that PID assuming that the handler runs in the context of the notification server. This way the notification server grants the privilege to the Mozilla app. We'll try this soon.

Comment 5

[betterbird.project+12]

We'll try this soon.

No dice at all, this isn't even being compiled for TB:
https://searchfox.org/mozilla-central/rev/47e291435c52512f0c664a89abaf1dcab01c2f69/toolkit/mozapps/defaultagent/Notification.cpp#513

Side note: The "old method" (alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled false) worked in 102 and brought the window to the foreground, in 115 even that doesn't work any more.

Comment 6

[Nick Rishel [:nrishel]]

I'm not sure I have the full context here but if TB is using WinToast for notifications then you're outside of the context of Gecko's native nsIAlert notifications. In general we're on track to phase out our use of WinToast, which is exclusively used in the Default Agent which was previously a standalone exe.

There's WIP documentation explaining the not-intuitive control flow of notifications on Windows. The jist is that handling for notification callbacks is split across two executable: a DLL Surrogate using our NotificationServer COM DLL and Firefox's in-memory notification callbacks. This split is necessary as the COM surrogate will reinvoke Firefox when it is closed before a user interacts with the notification, when alerts.useSystemBackend.windows.notificationserver.enabled is false the DLL Surrogate early exits and the application won't be restarted.

The DLL Surrogate also needs to pass Foreground permissions to Gecko, whether these permissions can be passed along is heuristic in nature and seems to rely on the receiving process having an event loop/window. This presents an issue when interacting with the Remote Server which will instead pass along the command line to a running Gecko instance and exit, because it breaks the heuristic chain in some way. To deal with this we have the final running Gecko instance which will handle the notification communicate back to the DLL Surrogate via a pipe it's PID, which the notification server then grants foreground permission to.

A note for anyone testing, SetForegroundWindow frustratingly always succeeds when run under a debugger. Also an application which doesn't have foreground permissions can not grant or set foreground permissions, so passing around the PID at time of notification creation won't work if we want to e.g. set a relaunched gecko instance to the foreground.

With that context out of the way:

1. Any insights re. the activation delay?
2. Does the SetForegroundWindow() at ToastNotificationHandler.cpp#879 work in FF or does it fail silently?
   It works, see above.

If the notification server is enabled via alerts.useSystemBackend.windows.notificationserver.enabled, the notification server has a 10 second timeout before it exits the notification handler so it can pass foreground permissions to the receiving gecko process. If enabled but the PID is never passed back, we'd expect this hitch.

3. In ToastNotification.cpp#609 (from bug 1795974) you allude to (quote) "Send our window's PID to the notification server", but is that actually done? The PID is a local variable, there is no call to AllowSetForegroundWindow() we can see anywhere. Besides, ToastNotification::SignalComNotificationHandled() is only called as part of ToastNotification::HandleWindowsTag().

When the notification server is enabled it always creates a new gecko process, which results in a new command line being passed either because a new gecko process was created or because the remote server passes it into an existing process. Therefore SignalComNotificationHandled will always be called (unless if the remote server is disabled but we don't support that use case).

Comment 7

[betterbird.project+12]

Maybe it's best to take a step back and look at the context.

TB is using nsIAlert here:
https://searchfox.org/comm-central/rev/f9b5e9d86828d56563cf6dff61ca001230931769/mailnews/base/src/MailNotificationManager.jsm#341-367
if the user has chosed to use "system alerts". If not, a XUL/XHTML window is shown.

We believe there is no requirement to launch TB when the user closed TB and hasn't clicked onto the notification. On the other hand, the delay of 10 seconds is not useful since the user is used to clicking onto the notification and getting their message shown.

The observed behaviour for windows system notifications/alerts is this:
In TB 102 everything works.
In TB 115 the notification server is not included, missing back port of bug 1838139 and bug 1848232 to ESR 115.
TB has set alerts.useSystemBackend to false by default, but alerts.useSystemBackend.windows.notificationserver.enabled to true which leads to total malfunction (bug 1858603, bug 1840123).
With alerts.useSystemBackend.windows.notificationserver.enabled also set to false manually, 115 works as 102, so the activation via the notification-click is immediate, minimised windows are restored, but non-minimised windows aren't raised due to a failing SetForegroundWindow(). We've experimented putting the 102 code in ToastNotification[Handler].{cpp|h} into the 115 tree and that in fact restores 102 behaviour. So there must be one change between 102 and 115 which damages the SetForegroundWindow().

In TB's current trunk the notification server is included, so there are two options:
alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled both at false. That gives
115 behaviour, working, but no SetForegroundWindow().

alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled both at true. That gives
the behaviour reported in comment #0: working with 10 sec delay, but also no SetForegroundWindow().

So where does that leave TB? As the 10 second delay is not desired and there is no requirement to relaunch, should TB pursue alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled both at false?

If so, do you have any hint why in this combination the SetForegroundWindow() still fails?

BTW, yes, the debugging quirk of SetForegroundWindow() succeeding during debugging has been documented here (quote):
Either the foreground process or the calling process is being debugged.

We're not setting NI again to avoid further noise but we'd appreciate your advice.

Comment 8

[Nick Rishel [:nrishel]]

The least brittle approach IMO is to opt into the relaunch behavior and find some sensible action to take.

The second approach would be to in some way disable the notification server registration from both the installer and during runtime. I don't recommend this approach because you'll also need to remove existing registrations (messy), and you'll need to change from the Toast-specific AUMID to the Start Menu shortcut AUMID (this will introduce an unfixable bug for users who opt out of a Start Menu entry). I suspect the fastest way to resolve this issue is adopting and adapting to the new notification server.

In TB 102 everything works.
In TB 115 the notification server is not included, missing back port of bug 1838139 and bug 1848232 to ESR 115.

I believe the notification server changed landed in Fx110.

Comment 9

[betterbird.project+12]

(In reply to Nick Rishel [:nrishel] from comment #8)

The least brittle approach IMO is to opt into the relaunch behavior and find some sensible action to take.

OK, that means alerts.useSystemBackend and alerts.useSystemBackend.windows.notificationserver.enabled both at true and fixing/adjusting the 10 seconds activation delay and the SetForegroundWindow() issue as originally reported. With all due respect, we don't believe there is anyone on the TB team capable of doing this since all this is very close to Windows system programming and the required code changes are in the Mozilla platform code.

Comment 10

[Ben Jordan]

Attachment: same problem

Thanks from lihyheles1973.

Comment 11

[Alexander Kriegisch]

Actually, I am not so sure what TB 120 means, if the current release is 115.8.1 for me as a regular user. I simply know that the notifications are useless, because clicking on the does nothing on Windows 10 Pro. This seems like a major issue to me. Why was it not fixed yet? Can somebody explain in a way understandable for simple users? (I am a software developer, but regarding TB I am just a simple user using a an e-mail client.)

Comment 12

[Francesco]

TB 120 is a beta version. The bug related to TB 115 is bug 1858603.

Comment 13

[Alessandro Castellani [:aleca]]

I'm fixing a few tiny things in bug 1893349 in order to remove the 10 seconds delay and fix the display of the message after clicking on the notification.
Once that's done, we will continue the work in this bug to fix the SetForegroundWindow() not working for Thunderbird.

Comment 14

[Alessandro Castellani [:aleca]]

Continuing the work here after more info and some suggestions reported in bug 1893349 comment 12

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: Bug 1863798 - Fix bringing the app to the foreground when clicking on native Windows notifications. r=BenC,darktrojan,mkmelin

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1863798 - Fix bringing the app to the foreground when clicking on native Windows notifications. r=aleca

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8e7090d4739e
Fix bringing the app to the foreground when clicking on native Windows notifications. r=aleca

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9404233 [details]
Bug 1863798 - Fix bringing the app to the foreground when clicking on native Windows notifications. r=aleca

[Approval Request Comment]
Approved for beta (bringing up some notification patches in advance of 128.0b1)

Comment 20

[Daniel Darnell [:dandarnell]]

Thunderbird 127.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/7dac67c0ffaf

Comment 21

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello,

Managed to reproduce this issue with the affected build from 2023-11-08 (20231108105500) using Windows 11.

Confirming this issue as verified fixed on 127.0b5(20240603175044) using macOS 14, Windows 11 and Ubuntu 22.