Mash notifications: fix handling of out-of-order Display/Close pairs.

The linked bug uncovered a case where ShowClientNotification was called
multiple times with the same notification (i.e. the same notification
ID) on the Chrome side, while Ash sent a HandleNotificationClosed
for the first shown notification which arrived in Chrome after the
second ShowClientNotification. Then Ash subsequently sent a second
HandleNotificationClosed, but since the notification had already been
removed from the Chrome-side cache in response to the first
HandleNotificationClosed, an error was thrown (specifically, the check
in NotificationPlatformBridgeChromeOs::HandleNotificationClosed()).

The solution is to create a token that is separate from the
notification's ID to identify the Display call to which the Close is
paired. This token is not used for other callbacks, such as the button
handlers, because then Chrome could drop user input that occurred around
the same time as a notification being updated (seems fairly likely for
any notification that has a progress bar, such as downloads).

Bug: 825141
Change-Id: I01f66dcc1107bfa7337c447780edb897e24498e6
Reviewed-on: https://chromium-review.googlesource.com/979268
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548484}

ash/message_center/message_center_controller.cc

@@ -8,6 +8,7 @@
 #include "ash/public/cpp/vector_icons/vector_icons.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/command_line.h"
+#include "base/unguessable_token.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/notification.h"
@@ -44,11 +45,14 @@ class AshClientNotificationDelegate
     : public message_center::NotificationDelegate {
  public:
   AshClientNotificationDelegate(const std::string& notification_id,
+                                const base::UnguessableToken& display_token,
                                 mojom::AshMessageCenterClient* client)
-      : notification_id_(notification_id), client_(client) {}
+      : notification_id_(notification_id),
+        display_token_(display_token),
+        client_(client) {}
 
   void Close(bool by_user) override {
-    client_->HandleNotificationClosed(notification_id_, by_user);
+    client_->HandleNotificationClosed(display_token_, by_user);
   }
 
   void Click(const base::Optional<int>& button_index,
@@ -72,7 +76,12 @@ class AshClientNotificationDelegate
  private:
   ~AshClientNotificationDelegate() override = default;
 
-  std::string notification_id_;
+  // The ID of the notification.
+  const std::string notification_id_;
+
+  // The token that was generated for the ShowClientNotification() call.
+  const base::UnguessableToken display_token_;
+
   mojom::AshMessageCenterClient* client_;
 
   DISALLOW_COPY_AND_ASSIGN(AshClientNotificationDelegate);
@@ -139,12 +148,14 @@ void MessageCenterController::SetClient(
 }
 
 void MessageCenterController::ShowClientNotification(
-    const message_center::Notification& notification) {
+    const message_center::Notification& notification,
+    const base::UnguessableToken& display_token) {
   DCHECK(client_.is_bound());
   auto message_center_notification =
       std::make_unique<message_center::Notification>(notification);
-  message_center_notification->set_delegate(base::WrapRefCounted(
-      new AshClientNotificationDelegate(notification.id(), client_.get())));
+  message_center_notification->set_delegate(
+      base::WrapRefCounted(new AshClientNotificationDelegate(
+          notification.id(), display_token, client_.get())));
   MessageCenter::Get()->AddNotification(std::move(message_center_notification));
 }
 

ash/message_center/message_center_controller.h

@@ -38,7 +38,8 @@ class ASH_EXPORT MessageCenterController
   void SetClient(
       mojom::AshMessageCenterClientAssociatedPtrInfo client) override;
   void ShowClientNotification(
-      const message_center::Notification& notification) override;
+      const message_center::Notification& notification,
+      const base::UnguessableToken& display_token) override;
   void CloseClientNotification(const std::string& id) override;
   void UpdateNotifierIcon(const message_center::NotifierId& notifier_id,
                           const gfx::ImageSkia& icon) override;

ash/message_center/notifier_settings_view_unittest.cc

@@ -38,7 +38,8 @@ class TestAshMessageCenterClient : public mojom::AshMessageCenterClient {
   }
 
   // mojom::AshMessageCenterClient:
-  void HandleNotificationClosed(const std::string& id, bool by_user) override {}
+  void HandleNotificationClosed(const base::UnguessableToken& token,
+                                bool by_user) override {}
   void HandleNotificationClicked(const std::string& id) override {}
   void HandleNotificationButtonClicked(
       const std::string& id,

ash/public/interfaces/ash_message_center_controller.mojom

@@ -8,6 +8,7 @@ import "ui/gfx/image/mojo/image.mojom";
 import "ui/message_center/public/mojo/notification.mojom";
 import "ui/message_center/public/mojo/notifier_id.mojom";
 import "mojo/public/mojom/base/string16.mojom";
+import "mojo/public/mojom/base/unguessable_token.mojom";
 
 // A struct that contains information for presenting a notifier in a settings
 // panel.
@@ -33,8 +34,12 @@ struct NotifierUiData {
 interface AshMessageCenterController {
   SetClient(associated AshMessageCenterClient client);
 
-  ShowClientNotification(message_center.mojom.Notification notification);
+  // Shows a notification. |display_token| will be used to reference this
+  // notification for AshMessageCenterClient::Close().
+  ShowClientNotification(message_center.mojom.Notification notification,
+                         mojo_base.mojom.UnguessableToken display_token);
 
+  // Closes a notification by the notification ID.
   CloseClientNotification(string id);
 
   UpdateNotifierIcon(message_center.mojom.NotifierId notifier_id,
@@ -54,7 +59,8 @@ interface AshMessageCenterController {
 // the client.
 interface AshMessageCenterClient {
   // Called when a notification previously displayed by the client is closed.
-  HandleNotificationClosed(string id, bool by_user);
+  HandleNotificationClosed(mojo_base.mojom.UnguessableToken display_token,
+                           bool by_user);
 
   // Called when the body of a notification is clicked.
   HandleNotificationClicked(string id);

chrome/browser/notifications/chrome_ash_message_center_client.cc

@@ -6,6 +6,7 @@
 
 #include "ash/public/interfaces/constants.mojom.h"
 #include "base/i18n/string_compare.h"
+#include "base/stl_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/notifications/arc_application_notifier_controller_chromeos.h"
 #include "chrome/browser/notifications/extension_notifier_controller.h"
@@ -20,6 +21,9 @@ using message_center::NotifierId;
 
 namespace {
 
+// The singleton instance, which is tracked to allow access from tests.
+ChromeAshMessageCenterClient* g_chrome_ash_message_center_client = nullptr;
+
 // All notifier actions are performed on the notifiers for the currently active
 // profile, so this just returns the active profile.
 Profile* GetProfileForNotifiers() {
@@ -51,6 +55,9 @@ class NotifierComparator {
 ChromeAshMessageCenterClient::ChromeAshMessageCenterClient(
     NotificationPlatformBridgeDelegate* delegate)
     : delegate_(delegate), binding_(this) {
+  DCHECK(!g_chrome_ash_message_center_client);
+  g_chrome_ash_message_center_client = this;
+
   // May be null in unit tests.
   auto* connection = content::ServiceManagerConnection::GetForProcess();
   if (connection) {
@@ -77,7 +84,10 @@ ChromeAshMessageCenterClient::ChromeAshMessageCenterClient(
           new arc::ArcApplicationNotifierControllerChromeOS(this))));
 }
 
-ChromeAshMessageCenterClient::~ChromeAshMessageCenterClient() {}
+ChromeAshMessageCenterClient::~ChromeAshMessageCenterClient() {
+  DCHECK_EQ(this, g_chrome_ash_message_center_client);
+  g_chrome_ash_message_center_client = nullptr;
+}
 
 // The unused variables here will not be a part of the future
 // NotificationPlatformBridge interface.
@@ -86,7 +96,18 @@ void ChromeAshMessageCenterClient::Display(
     Profile* /* profile */,
     const message_center::Notification& notification,
     std::unique_ptr<NotificationCommon::Metadata> metadata) {
-  controller_->ShowClientNotification(notification);
+  // Remove any previous mapping to |notification.id()| before inserting a new
+  // one.
+  base::EraseIf(
+      displayed_notifications_,
+      [notification](
+          const std::pair<base::UnguessableToken, std::string>& pair) {
+        return pair.second == notification.id();
+      });
+
+  base::UnguessableToken token = base::UnguessableToken::Create();
+  displayed_notifications_[token] = notification.id();
+  controller_->ShowClientNotification(notification, token);
 }
 
 // The unused variable here will not be a part of the future
@@ -113,9 +134,13 @@ void ChromeAshMessageCenterClient::SetReadyCallback(
 }
 
 void ChromeAshMessageCenterClient::HandleNotificationClosed(
-    const std::string& id,
+    const base::UnguessableToken& display_token,
     bool by_user) {
-  delegate_->HandleNotificationClosed(id, by_user);
+  auto entry = displayed_notifications_.find(display_token);
+  if (entry != displayed_notifications_.end()) {
+    delegate_->HandleNotificationClosed(entry->second, by_user);
+    displayed_notifications_.erase(entry);
+  }
 }
 
 void ChromeAshMessageCenterClient::HandleNotificationClicked(
@@ -180,3 +205,8 @@ void ChromeAshMessageCenterClient::OnNotifierEnabledChanged(
   if (controller_)
     controller_->NotifierEnabledChanged(notifier_id, enabled);
 }
+
+// static
+void ChromeAshMessageCenterClient::FlushForTesting() {
+  g_chrome_ash_message_center_client->binding_.FlushForTesting();
+}

chrome/browser/notifications/chrome_ash_message_center_client.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_NOTIFICATIONS_CHROME_ASH_MESSAGE_CENTER_CLIENT_H_
 
 #include "ash/public/interfaces/ash_message_center_controller.mojom.h"
+#include "base/unguessable_token.h"
 #include "chrome/browser/notifications/notification_platform_bridge.h"
 #include "chrome/browser/notifications/notification_platform_bridge_chromeos.h"
 #include "chrome/browser/notifications/notifier_controller.h"
@@ -37,7 +38,8 @@ class ChromeAshMessageCenterClient : public NotificationPlatformBridge,
   void SetReadyCallback(NotificationBridgeReadyCallback callback) override;
 
   // ash::mojom::AshMessageCenterClient:
-  void HandleNotificationClosed(const std::string& id, bool by_user) override;
+  void HandleNotificationClosed(const base::UnguessableToken& display_token,
+                                bool by_user) override;
   void HandleNotificationClicked(const std::string& id) override;
   void HandleNotificationButtonClicked(
       const std::string& id,
@@ -55,9 +57,20 @@ class ChromeAshMessageCenterClient : public NotificationPlatformBridge,
   void OnNotifierEnabledChanged(const message_center::NotifierId& notifier_id,
                                 bool enabled) override;
 
+  // Flushs |binding_|.
+  static void FlushForTesting();
+
  private:
   NotificationPlatformBridgeDelegate* delegate_;
 
+  // A mapping from display token to notification ID. The display token is
+  // generated each time a notification is shown (even if a notification is
+  // displayed more than once). This allows |this| to drop out-of-order
+  // HandleNotificationClosed() calls (i.e. those that arrive after the
+  // notification has already been re-displayed/updated and refer to an earlier
+  // notification).
+  std::map<base::UnguessableToken, std::string> displayed_notifications_;
+
   // Notifier source for each notifier type.
   std::map<message_center::NotifierId::NotifierType,
            std::unique_ptr<NotifierController>>

chrome/browser/notifications/chrome_ash_message_center_client_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/chrome_ash_message_center_client.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/notifications/notification_display_service.h"
+#include "chrome/browser/notifications/notification_test_util.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "ui/message_center/public/cpp/notification_delegate.h"
+
+namespace {
+
+class ChromeAshMessageCenterClientBrowserTest : public InProcessBrowserTest {
+ public:
+  ChromeAshMessageCenterClientBrowserTest() = default;
+  ~ChromeAshMessageCenterClientBrowserTest() override = default;
+
+  // InProcessBrowserTest overrides.
+  void SetUpInProcessBrowserTestFixture() override {
+    scoped_feature_list_.InitWithFeatures({features::kNativeNotifications}, {});
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeAshMessageCenterClientBrowserTest);
+};
+
+class TestNotificationDelegate : public message_center::NotificationDelegate {
+ public:
+  TestNotificationDelegate() {}
+
+  void Wait() { run_loop_.Run(); }
+
+  void Close(bool by_user) override {
+    close_count_++;
+    run_loop_.Quit();
+  }
+
+  int close_count() const { return close_count_; }
+
+ private:
+  ~TestNotificationDelegate() override {}
+
+  base::RunLoop run_loop_;
+  int close_count_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TestNotificationDelegate);
+};
+
+// Regression test for https://crbug.com/825141 that verifies out-of-order
+// Display/Close pairs are handled correctly.
+IN_PROC_BROWSER_TEST_F(ChromeAshMessageCenterClientBrowserTest,
+                       DisplayCloseOrdering) {
+  auto delegate = base::MakeRefCounted<TestNotificationDelegate>();
+  const std::string id("notification_identifier");
+  message_center::Notification notification(
+      message_center::NOTIFICATION_TYPE_SIMPLE, id, base::string16(),
+      base::string16(), gfx::Image(), base::ASCIIToUTF16("display_source"),
+      GURL(),
+      message_center::NotifierId(message_center::NotifierId::SYSTEM_COMPONENT,
+                                 "notifier_id"),
+      {}, delegate);
+
+  auto* display_service =
+      NotificationDisplayService::GetForProfile(browser()->profile());
+  display_service->Display(NotificationHandler::Type::TRANSIENT, notification);
+  display_service->Close(NotificationHandler::Type::TRANSIENT,
+                         notification.id());
+  // The Close callback should be fired asynchronously, so there is no close
+  // yet.
+  EXPECT_EQ(0, delegate->close_count());
+
+  display_service->Display(NotificationHandler::Type::TRANSIENT, notification);
+  display_service->Close(NotificationHandler::Type::TRANSIENT,
+                         notification.id());
+  ChromeAshMessageCenterClient::FlushForTesting();
+
+  // Only one close logged because Display was called again before the first
+  // close arrived.
+  EXPECT_EQ(1, delegate->close_count());
+}
+
+}  // namespace

chrome/browser/notifications/notification_platform_bridge_chromeos.cc

@@ -48,7 +48,12 @@ void NotificationPlatformBridgeChromeOs::Display(
 void NotificationPlatformBridgeChromeOs::Close(
     Profile* profile,
     const std::string& notification_id) {
-  impl_->Close(profile, notification_id);
+  // TODO(estade): we need |is_incognito| here.
+  const std::string profile_notification_id =
+      ProfileNotification::GetProfileNotificationId(
+          notification_id, NotificationUIManager::GetProfileID(profile));
+
+  impl_->Close(nullptr, profile_notification_id);
 }
 
 void NotificationPlatformBridgeChromeOs::GetDisplayed(

chrome/test/BUILD.gn

@@ -1662,6 +1662,7 @@ test("browser_tests") {
         "../browser/extensions/api/screenlock_private/screenlock_private_apitest.cc",
         "../browser/extensions/api/vpn_provider/vpn_provider_apitest.cc",
         "../browser/mash_service_registry_browsertest.cc",
+        "../browser/notifications/chrome_ash_message_center_client_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",