MessageCenter: Fix crash on reentrant RemoveAll

If a NotificationDelegate calls RemoveNotification in the Close handler
we're trying to close it twice if the user clicks the "Close all" button
in the message center. Instead remove the Notification from the list
before calling Close so the next call exits early and is a noop as
expected.

This was fixed for RemoveNotification before in crrev.com/c/2330019 and
this CL implements the same fix for RemoveAllNotifications.

Bug: 1135709
Change-Id: I27e365957554c56a6cbea0200c408c6706189f9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2461119
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815592}

ui/message_center/message_center_impl.cc

@@ -293,9 +293,13 @@ void MessageCenterImpl::RemoveAllNotifications(bool by_user, RemoveType type) {
 
     ids.insert(notification->id());
     scoped_refptr<NotificationDelegate> delegate = notification->delegate();
+
+    // Remove notification before calling the Close method in case it calls
+    // RemoveNotification reentrantly.
+    notification_list_->RemoveNotification(notification->id());
+
     if (delegate.get())
       delegate->Close(by_user);
-    notification_list_->RemoveNotification(notification->id());
   }
 
   if (!ids.empty()) {

ui/message_center/message_center_impl_unittest.cc

@@ -208,6 +208,13 @@ class MessageCenterImplTest : public testing::Test {
         NOTIFICATION_TYPE_SIMPLE);
   }
 
+  std::unique_ptr<Notification> CreateSimpleNotificationWithDelegate(
+      const std::string& id,
+      scoped_refptr<NotificationDelegate> delegate) {
+    return CreateNotificationWithNotifierIdAndDelegate(
+        id, kDefaultAppId, NOTIFICATION_TYPE_SIMPLE, delegate);
+  }
+
   std::unique_ptr<Notification> CreateNotification(const std::string& id,
                                                    NotificationType type) {
     return CreateNotificationWithNotifierId(id, kDefaultAppId, type);
@@ -217,6 +224,15 @@ class MessageCenterImplTest : public testing::Test {
       const std::string& id,
       const std::string& notifier_id,
       NotificationType type) {
+    return CreateNotificationWithNotifierIdAndDelegate(
+        id, notifier_id, type, base::MakeRefCounted<TestDelegate>());
+  }
+
+  std::unique_ptr<Notification> CreateNotificationWithNotifierIdAndDelegate(
+      const std::string& id,
+      const std::string& notifier_id,
+      NotificationType type,
+      scoped_refptr<NotificationDelegate> delegate) {
     RichNotificationData optional_fields;
     optional_fields.buttons.emplace_back(UTF8ToUTF16("foo"));
     optional_fields.buttons.emplace_back(UTF8ToUTF16("foo"));
@@ -224,7 +240,7 @@ class MessageCenterImplTest : public testing::Test {
         type, id, UTF8ToUTF16("title"), UTF8ToUTF16(id),
         gfx::Image() /* icon */, base::string16() /* display_source */, GURL(),
         NotifierId(NotifierType::APPLICATION, notifier_id), optional_fields,
-        base::MakeRefCounted<TestDelegate>());
+        delegate);
   }
 
   TestDelegate* GetDelegate(const std::string& id) const {
@@ -998,15 +1014,11 @@ TEST_F(MessageCenterImplTest, RemoveNonVisibleNotification) {
 }
 
 TEST_F(MessageCenterImplTest, RemoveInCloseHandler) {
-  std::string id("id1");
+  const std::string id("id1");
 
   // Create a notification that calls RemoveNotification() on close.
-  auto notification = std::make_unique<Notification>(
-      NOTIFICATION_TYPE_SIMPLE, id, UTF8ToUTF16("title"), UTF8ToUTF16(id),
-      gfx::Image() /* icon */, base::string16() /* display_source */, GURL(),
-      NotifierId(NotifierType::APPLICATION, kDefaultAppId),
-      RichNotificationData(),
-      base::MakeRefCounted<DeleteOnCloseDelegate>(message_center(), id));
+  auto notification = CreateSimpleNotificationWithDelegate(
+      id, base::MakeRefCounted<DeleteOnCloseDelegate>(message_center(), id));
   message_center()->AddNotification(std::move(notification));
   EXPECT_TRUE(message_center()->FindVisibleNotificationById(id));
 
@@ -1015,6 +1027,29 @@ TEST_F(MessageCenterImplTest, RemoveInCloseHandler) {
   EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
 }
 
+// Regression test for https://crbug.com/1135709
+TEST_F(MessageCenterImplTest, RemoveInCloseHandlerCloseAll) {
+  const std::string id1("id1");
+  const std::string id2("id2");
+
+  // Create two notifications that call RemoveNotification() on close.
+  auto notification1 = CreateSimpleNotificationWithDelegate(
+      id1, base::MakeRefCounted<DeleteOnCloseDelegate>(message_center(), id1));
+  auto notification2 = CreateSimpleNotificationWithDelegate(
+      id2, base::MakeRefCounted<DeleteOnCloseDelegate>(message_center(), id2));
+  message_center()->AddNotification(std::move(notification1));
+  message_center()->AddNotification(std::move(notification2));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id1));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id2));
+
+  // Then remove all notifications which calls RemoveNotification() reentrantly.
+  message_center()->RemoveAllNotifications(
+      true /* by_user */,
+      message_center::MessageCenter::RemoveType::NON_PINNED);
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id1));
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id2));
+}
+
 TEST_F(MessageCenterImplTest, FindNotificationsByAppId) {
   message_center()->SetHasMessageCenterView(true);