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/1058491Reviewed-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}

chrome/browser/browser_process_impl.cc

@@ -135,6 +135,7 @@
 #include "services/preferences/public/cpp/in_process_service_factory.h"
 #include "ui/base/idle/idle.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/ui_base_features.h"
 #include "ui/message_center/message_center.h"
 
 #if defined(OS_WIN)
@@ -676,11 +677,17 @@ NotificationUIManager* BrowserProcessImpl::notification_ui_manager() {
 // are enabled by default.
 #if defined(OS_ANDROID)
   return nullptr;
-#else
+#else  // !defined(OS_ANDROID)
+#if defined(OS_CHROMEOS)
+  if (base::FeatureList::IsEnabled(features::kNativeNotifications) ||
+      base::FeatureList::IsEnabled(features::kMash)) {
+    return nullptr;
+  }
+#endif  // defined(OS_CHROMEOS)
   if (!created_notification_ui_manager_)
     CreateNotificationUIManager();
   return notification_ui_manager_.get();
-#endif
+#endif  // !defined(OS_ANDROID)
 }
 
 NotificationPlatformBridge* BrowserProcessImpl::notification_platform_bridge() {

chrome/browser/notifications/message_center_notifications_browsertest.cc

@@ -12,12 +12,14 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/notifications/message_center_notification_manager.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/keep_alive_registry/keep_alive_registry.h"
 #include "components/keep_alive_registry/keep_alive_types.h"
@@ -62,7 +64,9 @@ class TestAddObserver : public message_center::MessageCenterObserver {
 
 class MessageCenterNotificationsTest : public InProcessBrowserTest {
  public:
-  MessageCenterNotificationsTest() {}
+  MessageCenterNotificationsTest() {
+    feature_list_.InitAndDisableFeature(features::kNativeNotifications);
+  }
 
   MessageCenterNotificationManager* manager() {
     return static_cast<MessageCenterNotificationManager*>(
@@ -140,6 +144,8 @@ class MessageCenterNotificationsTest : public InProcessBrowserTest {
     base::RunLoop loop;
     loop.RunUntilIdle();
   }
+
+  base::test::ScopedFeatureList feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest, RetrieveBaseParts) {

chrome/browser/notifications/notification_platform_bridge_chromeos.cc

@@ -4,15 +4,48 @@
 
 #include "chrome/browser/notifications/notification_platform_bridge_chromeos.h"
 
+#include "base/callback_list.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/notifications/chrome_ash_message_center_client.h"
+#include "chrome/browser/notifications/notification_display_service_factory.h"
 #include "chrome/browser/notifications/notification_display_service_impl.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_icon_loader.h"
+#include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
 #include "ui/gfx/image/image.h"
 
+namespace {
+
+// This class informs NotificationPlatformBridgeChromeOs, which is a singleton,
+// when a profile is being destroyed. This allows the bridge to notify
+// delegates/handlers of the close and remove the notification before the
+// profile is destroyed (otherwise Ash might asynchronously notify the bridge of
+// operations on a notification associated with a profile that has already been
+// destroyed).
+class ProfileShutdownNotifier
+    : public BrowserContextKeyedServiceShutdownNotifierFactory {
+ public:
+  static ProfileShutdownNotifier* GetInstance() {
+    return base::Singleton<ProfileShutdownNotifier>::get();
+  }
+
+ private:
+  friend struct base::DefaultSingletonTraits<ProfileShutdownNotifier>;
+
+  ProfileShutdownNotifier()
+      : BrowserContextKeyedServiceShutdownNotifierFactory(
+            "NotificationDisplayService") {
+    DependsOn(NotificationDisplayServiceFactory::GetInstance());
+  }
+  ~ProfileShutdownNotifier() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(ProfileShutdownNotifier);
+};
+
+}  // namespace
+
 // static
 NotificationPlatformBridge* NotificationPlatformBridge::Create() {
   return new NotificationPlatformBridgeChromeOs();
@@ -34,6 +67,15 @@ void NotificationPlatformBridgeChromeOs::Display(
     Profile* profile,
     const message_center::Notification& notification,
     std::unique_ptr<NotificationCommon::Metadata> metadata) {
+  if (profile_shutdown_subscriptions_.find(profile) ==
+      profile_shutdown_subscriptions_.end()) {
+    profile_shutdown_subscriptions_[profile] =
+        ProfileShutdownNotifier::GetInstance()->Get(profile)->Subscribe(
+            base::BindRepeating(
+                &NotificationPlatformBridgeChromeOs::OnProfileDestroying,
+                base::Unretained(this), profile));
+  }
+
   auto active_notification = std::make_unique<ProfileNotification>(
       profile, notification, notification_type);
   impl_->Display(active_notification->notification());
@@ -75,7 +117,8 @@ void NotificationPlatformBridgeChromeOs::HandleNotificationClosed(
     const std::string& id,
     bool by_user) {
   auto iter = active_notifications_.find(id);
-  DCHECK(iter != active_notifications_.end());
+  if (iter == active_notifications_.end())
+    return;
   ProfileNotification* notification = iter->second.get();
 
   if (notification->type() == NotificationHandler::Type::TRANSIENT) {
@@ -93,6 +136,9 @@ void NotificationPlatformBridgeChromeOs::HandleNotificationClosed(
 void NotificationPlatformBridgeChromeOs::HandleNotificationClicked(
     const std::string& id) {
   ProfileNotification* notification = GetProfileNotification(id);
+  if (!notification)
+    return;
+
   if (notification->type() == NotificationHandler::Type::TRANSIENT) {
     notification->notification().delegate()->Click(base::nullopt,
                                                    base::nullopt);
@@ -111,6 +157,9 @@ void NotificationPlatformBridgeChromeOs::HandleNotificationButtonClicked(
     int button_index,
     const base::Optional<base::string16>& reply) {
   ProfileNotification* notification = GetProfileNotification(id);
+  if (!notification)
+    return;
+
   if (notification->type() == NotificationHandler::Type::TRANSIENT) {
     notification->notification().delegate()->Click(button_index, reply);
   } else {
@@ -125,6 +174,9 @@ void NotificationPlatformBridgeChromeOs::HandleNotificationButtonClicked(
 void NotificationPlatformBridgeChromeOs::
     HandleNotificationSettingsButtonClicked(const std::string& id) {
   ProfileNotification* notification = GetProfileNotification(id);
+  if (!notification)
+    return;
+
   if (notification->type() == NotificationHandler::Type::TRANSIENT) {
     notification->notification().delegate()->SettingsClick();
   } else {
@@ -140,6 +192,9 @@ void NotificationPlatformBridgeChromeOs::
 void NotificationPlatformBridgeChromeOs::DisableNotification(
     const std::string& id) {
   ProfileNotification* notification = GetProfileNotification(id);
+  if (!notification)
+    return;
+
   DCHECK_NE(NotificationHandler::Type::TRANSIENT, notification->type());
   NotificationDisplayServiceImpl::GetForProfile(notification->profile())
       ->ProcessNotificationOperation(NotificationCommon::DISABLE_PERMISSION,
@@ -152,6 +207,20 @@ void NotificationPlatformBridgeChromeOs::DisableNotification(
 ProfileNotification* NotificationPlatformBridgeChromeOs::GetProfileNotification(
     const std::string& profile_notification_id) {
   auto iter = active_notifications_.find(profile_notification_id);
-  DCHECK(iter != active_notifications_.end());
+  if (iter == active_notifications_.end())
+    return nullptr;
   return iter->second.get();
 }
+
+void NotificationPlatformBridgeChromeOs::OnProfileDestroying(Profile* profile) {
+  std::list<std::string> ids_to_close;
+  for (const auto& iter : active_notifications_) {
+    if (iter.second->profile() == profile)
+      ids_to_close.push_back(iter.second->notification().id());
+  }
+
+  for (auto id : ids_to_close)
+    HandleNotificationClosed(id, false);
+
+  profile_shutdown_subscriptions_.erase(profile);
+}

chrome/browser/notifications/notification_platform_bridge_chromeos.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "chrome/browser/notifications/notification_platform_bridge.h"
 #include "chrome/browser/notifications/profile_notification.h"
+#include "components/keyed_service/core/keyed_service_shutdown_notifier.h"
 
 class ChromeAshMessageCenterClient;
 
@@ -76,9 +77,16 @@ class NotificationPlatformBridgeChromeOs
   void DisableNotification(const std::string& id) override;
 
  private:
+  // Gets the ProfileNotification for the given identifier which has been
+  // mutated to uniquely identify the profile. This may return null if the
+  // notification has already been closed due to profile shutdown. Ash may
+  // asynchronously inform |this| of actions on notificationafter their
+  // associated profile has already been destroyed.
   ProfileNotification* GetProfileNotification(
       const std::string& profile_notification_id);
 
+  void OnProfileDestroying(Profile* profile);
+
   std::unique_ptr<ChromeAshMessageCenterClient> impl_;
 
   // A container for all active notifications, where IDs are permuted to
@@ -87,6 +95,10 @@ class NotificationPlatformBridgeChromeOs
   std::map<std::string, std::unique_ptr<ProfileNotification>>
       active_notifications_;
 
+  std::map<Profile*,
+           std::unique_ptr<KeyedServiceShutdownNotifier::Subscription>>
+      profile_shutdown_subscriptions_;
+
   DISALLOW_COPY_AND_ASSIGN(NotificationPlatformBridgeChromeOs);
 };
 

chrome/browser/notifications/notification_platform_bridge_chromeos_browsertest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/notifications/notification_platform_bridge_chromeos.h"
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/notifications/notification_display_service.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "ui/message_center/public/cpp/notification.h"
+#include "ui/message_center/public/cpp/notification_delegate.h"
+
+class NotificationPlatformBridgeChromeOsBrowserTest
+    : public InProcessBrowserTest,
+      public message_center::NotificationObserver {
+ public:
+  NotificationPlatformBridgeChromeOsBrowserTest() = default;
+  ~NotificationPlatformBridgeChromeOsBrowserTest() override {
+    EXPECT_EQ(expected_close_count_, close_count_);
+  }
+
+  // message_center::NotificationObserver
+  void Close(bool by_user) override { ++close_count_; }
+
+ protected:
+  int expected_close_count_ = 0;
+  int close_count_ = 0;
+
+  base::WeakPtrFactory<NotificationPlatformBridgeChromeOsBrowserTest>
+      weak_ptr_factory_{this};
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NotificationPlatformBridgeChromeOsBrowserTest);
+};
+
+// Tests that a notification delegate is informed of the notification closing
+// when it's closed due to shutdown.
+IN_PROC_BROWSER_TEST_F(NotificationPlatformBridgeChromeOsBrowserTest,
+                       Shutdown) {
+  message_center::Notification notification(
+      message_center::NOTIFICATION_TYPE_SIMPLE, "notification-id",
+      base::string16(), base::string16(), gfx::Image(), base::string16(),
+      GURL(), message_center::NotifierId(),
+      message_center::RichNotificationData(),
+      base::MakeRefCounted<message_center::ThunkNotificationDelegate>(
+          weak_ptr_factory_.GetWeakPtr()));
+
+  NotificationDisplayService::GetForProfile(browser()->profile())
+      ->Display(NotificationHandler::Type::TRANSIENT, notification);
+  expected_close_count_ = 1;
+}

chrome/test/BUILD.gn

@@ -1677,6 +1677,7 @@ test("browser_tests") {
         "../browser/extensions/api/vpn_provider/vpn_provider_apitest.cc",
         "../browser/extensions/chromeos_component_extensions_browsertest.cc",
         "../browser/notifications/chrome_ash_message_center_client_browsertest.cc",
+        "../browser/notifications/notification_platform_bridge_chromeos_browsertest.cc",
         "../browser/resources/chromeos/zip_archiver/test/zip_archiver_jstest.cc",
         "../browser/signin/chromeos_mirror_account_consistency_browsertest.cc",
         "../browser/ui/app_list/app_list_browsertest.cc",