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

commit 48b4285e027e0750756c491610a0c988054ffebf
Author: Evan Stade <estade@chromium.org>
Date: Wed May 16 17:52:46 2018

Chrome OS: Close outstanding notifications on shutdown

Previously we used the deprecated NotificationUiService::CancelAll
via BrowserCloseManager to do this. That used the MessageCenter
singleton directly, which worked with in-process Ash but not oop Ash.
By listening for profile shutdown in the NotificationPlatformBridge, we
can accomplish the same thing and it works in oop Ash as well.

This is a roundabout way of fixing the bug, which AFAICT is a race
between the Profile object shutting down and Ash asynchronously (via
mojo) notifying of a notification toast closing. I couldn't figure out
how to trigger this race, hence no direct test.

Bug:  842705 
Change-Id: I5913bcb3077450433e26ba6cc7397e17211bc9a6
Reviewed-on: https://chromium-review.googlesource.com/1058491
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559175}
[modify] https://crrev.com/48b4285e027e0750756c491610a0c988054ffebf/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/48b4285e027e0750756c491610a0c988054ffebf/chrome/browser/notifications/message_center_notifications_browsertest.cc
[modify] https://crrev.com/48b4285e027e0750756c491610a0c988054ffebf/chrome/browser/notifications/notification_platform_bridge_chromeos.cc
[modify] https://crrev.com/48b4285e027e0750756c491610a0c988054ffebf/chrome/browser/notifications/notification_platform_bridge_chromeos.h
[add] https://crrev.com/48b4285e027e0750756c491610a0c988054ffebf/chrome/browser/notifications/notification_platform_bridge_chromeos_browsertest.cc
[modify] https://crrev.com/48b4285e027e0750756c491610a0c988054ffebf/chrome/test/BUILD.gn

Yoshiki-san, I'll be out of the office for the next few days. I think it would be ideal to merge this to M67, but it should probably have at least a day to bake before requesting a merge. Would you be willing to take over merging it to M67 for me? I would greatly appreciate your help.

Alternatively, we could simply accept the crash as part of M67 since it should be rare and it only happens on shutdown. I am just a little worried about subtle breakages to notification behavior, e.g. web apps would not get a close event for notifications closed by shutdown, although I have a hard time imagining cases where that's of much importance, especially given that (I think) Chrome with native notification centers would frequently leave an app hanging, i.e. with no notification close event when the browser's shutting down.

+kbleicher for pre-merge-request heads up.

Evan, thank you for fixing this! I take over the work.

I think it's better to have this even in M67, but defer to TPM.

This bug requires manual review: We are only 10 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Reviewing merges for Kevin, do you have information requested in #4? Has this baked and what testing has been completed? Thank you.

No same crash is observed after this change is landed at 68.0.3433.0 per the crash dashboard. But I didn't repro this crash itself and can't test it manually. Anyway, I think this CL looks safe and is basically just adding null-checks and the code to clean-up notifications on destroying profile.

As Evan said, it's not mandatory and we might accept the crash. But I think it's worth to merge it since the crash rate is not low. Final decision is up to the TLM.

Merge request approved, M67 Chrome OS.

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

commit 9b16ac28d9034e15d3b1c54c43116a49799f4c78
Author: Evan Stade <estade@chromium.org>
Date: Wed May 23 20:02:33 2018

Chrome OS: Close outstanding notifications on shutdown

Previously we used the deprecated NotificationUiService::CancelAll
via BrowserCloseManager to do this. That used the MessageCenter
singleton directly, which worked with in-process Ash but not oop Ash.
By listening for profile shutdown in the NotificationPlatformBridge, we
can accomplish the same thing and it works in oop Ash as well.

This is a roundabout way of fixing the bug, which AFAICT is a race
between the Profile object shutting down and Ash asynchronously (via
mojo) notifying of a notification toast closing. I couldn't figure out
how to trigger this race, hence no direct test.

TBR=estade@chromium.org

(cherry picked from commit 48b4285e027e0750756c491610a0c988054ffebf)

Bug:  842705 
Change-Id: I5913bcb3077450433e26ba6cc7397e17211bc9a6
Reviewed-on: https://chromium-review.googlesource.com/1058491
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#559175}
Reviewed-on: https://chromium-review.googlesource.com/1070531
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#688}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/9b16ac28d9034e15d3b1c54c43116a49799f4c78/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/9b16ac28d9034e15d3b1c54c43116a49799f4c78/chrome/browser/notifications/message_center_notifications_browsertest.cc
[modify] https://crrev.com/9b16ac28d9034e15d3b1c54c43116a49799f4c78/chrome/browser/notifications/notification_platform_bridge_chromeos.cc
[modify] https://crrev.com/9b16ac28d9034e15d3b1c54c43116a49799f4c78/chrome/browser/notifications/notification_platform_bridge_chromeos.h
[add] https://crrev.com/9b16ac28d9034e15d3b1c54c43116a49799f4c78/chrome/browser/notifications/notification_platform_bridge_chromeos_browsertest.cc
[modify] https://crrev.com/9b16ac28d9034e15d3b1c54c43116a49799f4c78/chrome/test/BUILD.gn

thanks to Cindy and Yoshiki. Fingers crossed this is fixed.