I think this behavior is undesired as we don't have this issue in non-native notification. 

Peter, do you have any insights on this? Thanks!

To me it looks like a SetForeground call is made but Windows isn't letting Chrome into the foreground.

At first I thought this was because of ForegroundLockTimeout...
https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-2000-server/cc957208(v=technet.10)

... but this value is set for 200000 milliseconds on my machine (3 mins 20 sec) and I waited for over 3 and a half minutes before activating the notification but it still wouldn't show.

There's an interesting article on the ability to become foreground window here:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms632668(v=vs.85).aspx

Of particular interest is the fact that if Chrome is running under a debugger, it can set the foreground window, so to repro this on a tip-of-tree (and not just Canary) you need to make sure Chrome is not running under Visual Studio/WinDbg.

Looking at this some more I think it might be helpful to get insight from Greg/Rob...

Psychic debugging:

The chrome.exe process spun up by the notification center has the power to bring its own windows to the foreground, but it has no UX since it delegates to the existing browser process. To make this work, have the chrome.exe that delegates to the running browser call ::AllowSetForegroundWindow with the PID of the running browser process.

I concur with the psychic debugging analysis.

I implemented Greg's suggestion in https://chromium-review.googlesource.com/c/chromium/src/+/1043387, but it doesn't work.

I added a ::AllowSetForegroundWindow(process_singleton_->notified_pid()) call in chrome_browser_main.cc. I can confirm (via process monitor) that this call is from the chrome process that is launched by notification_helper, and the supplied pid is the one of the running browser process. Am I missing anything?

I am stuck here. Any thoughts please?

I left a couple of comments on the review. 
But it would be good to get input from Greg and/or Rob...

The AllowSetForegroundWindow call didn't succeed, error code 5 -- ERROR_ACCESS_DENIED.

From msdn:

The system restricts which processes can set the foreground window. A process can set the foreground window only if one of the following conditions is true:

The process is the foreground process.
The process was started by the foreground process.
The process received the last input event.
There is no foreground process.
The foreground process is being debugged.
The foreground is not locked (see LockSetForegroundWindow).
The foreground lock time-out has expired (see SPI_GETFOREGROUNDLOCKTIMEOUT in SystemParametersInfo).
No menus are active.

I am OOO for the next 4 weeks. Finnur, could you please take over? If you want to find someone else to work on this, Bruce may be able to help.

As it turns out, my suggestion would have been great if we weren't already doing it in chrome::AttemptToNotifyRunningChrome. :-(

In that case, I'm not sure why it isn't working. Maybe there's something odd about the way that the NC launches chrome.exe so that it isn't actually given permission to foreground itself.

Are other desktop apps somehow able to do this?

Does Chrome's notification process need to call this as well? That is, do we need to have the notification process pass this privilege along to chrome.exe so that chrome.exe can pass it on to another chrome.exe process?

I did a test a few days back: Obtain the pid for the chrome.exe that notification_helper.exe starts, but AllowSetForegroundWindow(pid) results in error 5.

Oh right. We have yet another process in the loop. That's going to be hella-racy -- there's no point at which the spawned chrome.exe will stop and wait to be given fg permission and then continue to pass it along to the running browser process.

We're counting on the notification center calling CoAllowSetForegroundWindow before it calls our INotificationActivationCallback::Activate. I wonder if/how we can verify that it's really doing this.

I asked Microsoft and got confirmation that CoAllowSetForegroundWindow is being called (as of build 16299, which I'm testing on).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/933cf169043979b8fc594877d0cfe20dbd6ffe27

commit 933cf169043979b8fc594877d0cfe20dbd6ffe27
Author: Finnur Thorarinsson <finnur@chromium.org>
Date: Tue May 08 20:57:16 2018

Win Native Notifications: Fix window activation.

This fixes the issue with window activation not functioning correctly
when notifications are activated while Chrome is running but in the
background.

Bug:  837796 ,  734095 
Change-Id: I8eac14403850a13d60e7a50c3c795fed96205d7e
Reviewed-on: https://chromium-review.googlesource.com/1049984
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556955}
[modify] https://crrev.com/933cf169043979b8fc594877d0cfe20dbd6ffe27/chrome/notification_helper/notification_activator.cc

The fix has been submitted. We're investigating whether there's a possibility for a race here and discussing possible mitigation if so. But on the whole this should be fixed and follow-up improvements can appear later if need be.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b260a210beabbbcf0cfc449188208dedaf92fa2

commit 2b260a210beabbbcf0cfc449188208dedaf92fa2
Author: Greg Thompson <grt@chromium.org>
Date: Wed May 09 08:50:04 2018

Revert "Win Native Notifications: Fix window activation."

This reverts commit 933cf169043979b8fc594877d0cfe20dbd6ffe27.

Reason for revert: Handle leak causing test failures.

Original change's description:
> Win Native Notifications: Fix window activation.
> 
> This fixes the issue with window activation not functioning correctly
> when notifications are activated while Chrome is running but in the
> background.
> 
> Bug:  837796 ,  734095 
> Change-Id: I8eac14403850a13d60e7a50c3c795fed96205d7e
> Reviewed-on: https://chromium-review.googlesource.com/1049984
> Reviewed-by: Greg Thompson <grt@chromium.org>
> Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556955}

TBR=finnur@chromium.org,grt@chromium.org

Change-Id: Ib0e0d132867555f822dc13061b63bca123c16df9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  837796 ,  734095 
Reviewed-on: https://chromium-review.googlesource.com/1051385
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557125}
[modify] https://crrev.com/2b260a210beabbbcf0cfc449188208dedaf92fa2/chrome/notification_helper/notification_activator.cc

Tested the issue on latest chrome 68.0.3425.0 using Windows 10 with steps mentioned below and still able to reproduce this issue on latest chrome 68.0.3425.0.

1) Launched chrome and Enabled win10 native notification flag
2) Navigated to given URL> ""https://tests.peter.sh/notification-generator/"" and enabled ""Display the notifications"" Tab. 
3.Pushed chrome browser to background and pulled other tabs to foreground.
4. Clicked on notifications 
5.Observed that chrome browesr not become the foreground


@finnur :Please find the attached screencast for your reference and help us in confirming the fix.

Thanks!

Deleted: 837796.mp4 1.9 MB

	837796.mp4 1.9 MB View Download

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da40ea379decf0fefd8bb92c5a93d3f7015d7433

commit da40ea379decf0fefd8bb92c5a93d3f7015d7433
Author: Finnur Thorarinsson <finnur@chromium.org>
Date: Fri May 11 18:28:49 2018

Win Native Notifications: Fix window activation (2nd attempt).

This fixes the issue with window activation not functioning correctly
when notifications are activated while Chrome is running but in the
background.

Bug:  837796 ,  734095 
Change-Id: I69f5c358ba55fa997e1c3859d653638131f1394c
Reviewed-on: https://chromium-review.googlesource.com/1051708
Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557949}
[modify] https://crrev.com/da40ea379decf0fefd8bb92c5a93d3f7015d7433/chrome/browser/notifications/notification_helper_launches_chrome_unittest.cc
[modify] https://crrev.com/da40ea379decf0fefd8bb92c5a93d3f7015d7433/chrome/notification_helper/notification_activator.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/31f8df48a7c5bcc8050c636ff0fa60b2bcecbf04

commit 31f8df48a7c5bcc8050c636ff0fa60b2bcecbf04
Author: Finnur Thorarinsson <finnur@chromium.org>
Date: Thu May 17 13:23:24 2018

Win Native Notifications: Don't abort if AllowSetForegroundWindow fails.

It may have unwanted side-effects in the Action Center and is really
a benign failure.

Bug:  841475 ,  837796 ,  734095 
Change-Id: I61cc2736742a50de72fd5a5b9c87ae233ff9a5f0
Reviewed-on: https://chromium-review.googlesource.com/1061414
Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559508}
[modify] https://crrev.com/31f8df48a7c5bcc8050c636ff0fa60b2bcecbf04/chrome/browser/notifications/notification_helper_launches_chrome_unittest.cc
[modify] https://crrev.com/31f8df48a7c5bcc8050c636ff0fa60b2bcecbf04/chrome/notification_helper/notification_activator.cc

Your testing methodology looks good, but it looks like my fix got reverted before you tested. Can you try again?

Awesome! Thank you Finnur!

Is Windows Build 16299 required for this to work? If so, the tester will need to make sure that the test machine meets this requirement.

Yes, Windows version 16299 or later is required.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d326a008e79cb49bdc3871e08bf3fb8ef902b871

commit d326a008e79cb49bdc3871e08bf3fb8ef902b871
Author: Xi Cheng <chengx@chromium.org>
Date: Wed May 30 03:53:55 2018

Log execute status of NotificationActivator::Activate

The metric is used to help investigate the issue of empty launch id
string and verify the fix to the issue of window's not becoming
foreground.

Bug:  839942 ,  837796 ,  734095 
Change-Id: Ia52d685d0b2c866aa1d048a984bf7932e5fa52f8
Reviewed-on: https://chromium-review.googlesource.com/1077369
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Xi Cheng <chengx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562725}
[modify] https://crrev.com/d326a008e79cb49bdc3871e08bf3fb8ef902b871/chrome/notification_helper/notification_activator.cc
[modify] https://crrev.com/d326a008e79cb49bdc3871e08bf3fb8ef902b871/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/d326a008e79cb49bdc3871e08bf3fb8ef902b871/tools/metrics/histograms/histograms.xml

Tested the issue on latest chrome 69.0.3446.0 using Windows 10 with steps mentioned below and still able to reproduce this issue on latest chrome 69.0.3446.0.

1) Launched chrome and Enabled win10 native notification flag
2) Navigated to given URL> """"https://tests.peter.sh/notification-generator/"""" and enabled """"Display the notifications"""" Tab. 
3.Pushed chrome browser to background and pulled other tabs to foreground.
4. Clicked on notifications 
5.Observed that chrome browser not become the foreground


@finnur :Please find the attached screencast for your reference and help us in confirming the fix.

Thanks!

Deleted: 837796.mp4 3.7 MB

	837796.mp4 3.7 MB View Download

Re comment 27:

Windows version 16299 or later is required for the fix to work. Could you please check your Windows version by typing "winver" in the command prompt?

Just in case we hit this type of problem again...

I saw a tweet that talks about the official status on how you make sure that you can activate a window, and the reality. It turns out that there are lots of ways to make activation work:

https://twitter.com/paulcbetts/status/1026622482696945664

I'm not at all sure that we should use these techniques, but it's good to know about them.

 Issue 876764  has been merged into this issue.

Hi folks - we (MightyText) continue to see this problem in production in Chrome 68, Windows 10 - consistently. Major apps like Gmail, Google Calendar, WhatsApp all do not work (notification clicks), when Chrome is not in the foreground.

We sent this info to Peter Beverloo via email as well.

Repro steps on Win 10:

- Enable desktop notifications in a web-desktop app like Gmail, Google Calendar, WhatsApp
- Receive a notification (which are now Windows native) while you *aren't* in Chrome (e.g. be in another app like Skype, a Text Editor, etc)
- On click of the notification - nothing happens. Expected behavior: Click should switch the user into Chrome and take user to the right URL.
- We are noticing that Chrome is switching to the right URL, but the Windows OS is *not* switching the user into Chrome.
- We've been able to reproduce on 4 different machines running Windows 10 on Chrome 68
- Also able to reproduce with Notifications created using the Chrome Notifications API (Chrome Extensions).

Overall, we think this is a big deal because apps like WhatsApp, Gmail, GCal have billions of users and I'm sure many (most?) use Desktop Notifications.

Thanks for the information. According to Finnur as in comment #25, Windows version (OS build) 16299 and above is required for his fix to work. Could you check this info on the machines you tested?

Hi Cheng,

Our Windows build numbers are

17134.191
17134.228
16299.125

The functionality is broken on all 3 machines.

Thanks for the feedback! Re-open this bug then.

We will fix this bug ASAP.

Thanks Cheng and Peter and others....

We are happy to help QA this in our multiple Windows 10 machines. Let us know when the fix hits Canary :)

maneesh: Can you navigate (on the computer where you can repro the failure) to:

chrome://histograms

... and upload the results? Thanks.

Yes will have our developer Kevin upload it shortly.

Attaching the histograms results.

Deleted: Histograms.html 1.4 MB

Histograms.html 1.4 MB View Download

Thanks Kevin! Could you use Chrome Canary to generate the histograms again? 

Some histograms we need for investigation have not hit Stable channel yet.

Thanks!

Sure, here's the histogram from canary

Deleted: Histograms_canary.html 1.5 MB

Histograms_canary.html 1.5 MB View Download

Hi Xi,

I'm not seeing any of the histograms we added to NotificationActivator::Activate. I seem to recall some work to make crashes work from a utility process, I thought histograms was part of that, but wasn't sure.

They should show up in chrome://histograms, no?

Re Finnur: yes they should show up in chrome://histograms, but in a delayed and batched manner since those histograms are written to the disk before getting uploaded to the server. To see those *.NotificationHelperMetrics.* histograms, we will have wait a bit and refresh the chrome://histograms page. I think that is why they are missing from Kevin's histograms.

Re all: I just upgraded my corp Windows machine to OS build 16299, and tested both the Stable and the Canary channels. The fix landed as in comment #21 doesn't work on my machine neither. I am glad I can repro this issue so I can do some independent investigation. I looked up the Notifications.NotificationHelper.NotificationActivatorSecondaryStatus metric on my machine. It has a value of 2 (kAllowSetForegroundWindowFailed) which is expected. 

Cheng / Finnur - we (Kevin and team) are happy to look for *.NotificationHelperMetrics.* in the chrome://histograms again since it sounds like they get batched.  Let us know if we should check that, and how often

Thanks maneesh@, Kevin and the team! Since I can repro this issue on my machine, I should be good to go on my own now. 

Finnur, do you need more information from them?

Xi: Nope. I think we're good. It should be enough that you can repro -- which is great news, by the way. Thanks for looking into this! It would be interesting to see what ::GetLastError() returns after calling AllowSetForegroundWindow.

Xi, are you seeing the Chrome icon flash in the Task Bar when this fails?

Re Finnur: yea, the Chrome icon in the Task Bar flashed in my case.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3c45c46562ec1d99737516cc83aa7c7b916851f

commit a3c45c46562ec1d99737516cc83aa7c7b916851f
Author: Xi Cheng <chengx@chromium.org>
Date: Wed Aug 29 00:45:09 2018

Simulate generic down/up key events to make AllowSetForegroundWindow call succeed

Despite the fact that the Windows notification center grants the helper
permission to set the foreground window, the helper fails to pass the baton
to Chrome at an alarming rate; see  https://crbug.com/837796 .

Sending generic down/up key events seems to fix it.

Bug:  837796 ,  734095 
Change-Id: I9b590d5149df35cfcb8219b0a386c5a9cfdb8eb0
Reviewed-on: https://chromium-review.googlesource.com/1192416
Commit-Queue: Xi Cheng <chengx@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586968}
[modify] https://crrev.com/a3c45c46562ec1d99737516cc83aa7c7b916851f/chrome/notification_helper/notification_activator.cc

Able to reproduce the issue on chrome version 68.0.3410.0 (build without fix) as per the comment #0.
Verified the fix on Windows 10 using Chrome version # 70.0.3536.0.
Attaching screen-cast for reference.
Observed that " Chrome becomes the foreground app " 
The fix is working as expected, adding Verified labels

Thanks...!

Deleted: 837796.mp4 10.2 MB

	837796.mp4 10.2 MB View Download

#49: Thanks for verifying the fix!

maneesh@, kevin@ and the team: the fix as in #48 has hit Canary (70.0.3536.0). Please have a try and let us know if it works on your machines. Thanks!

This UMA metric data today leads me to believe this issue is now fixed in the latest Canary. This success rate is almost 100% now, while it was only <20% before.

If the issue comes back or the fix causes other issues, we can re-open this bug.

Many thanks to brucedawson@ who provided the twitter link as in #29, which is very helpful.

We just tested this behavior on a Windows 10 machine running Chrome Canary (v70.0.3537.0) and it looks like the issue has been fixed and works as expected now.