MessageCenter: Fix crash on reentrant RemoveNotification

If a NotificationDelegate calls RemoveNotification in the Close handler
we're currently ending up in an endless loop. Instead remove the
Notification from the list before calling Close so the next call exits
early and is a noop as expected.

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

ui/message_center/message_center_impl.cc

@@ -247,10 +247,14 @@ void MessageCenterImpl::RemoveNotification(const std::string& id,
 
   scoped_refptr<NotificationDelegate> delegate =
       notification_list_->GetNotificationDelegate(copied_id);
+
+  // Remove notification before calling the Close method in case it calls
+  // RemoveNotification reentrantly.
+  notification_list_->RemoveNotification(copied_id);
+
   if (delegate.get())
     delegate->Close(by_user);
 
-  notification_list_->RemoveNotification(copied_id);
   visible_notifications_ =
       notification_list_->GetVisibleNotifications(blockers_);
   for (auto& observer : observer_list_)

ui/message_center/message_center_impl_unittest.cc

@@ -136,6 +136,28 @@ class TestDelegate : public NotificationDelegate {
   DISALLOW_COPY_AND_ASSIGN(TestDelegate);
 };
 
+class DeleteOnCloseDelegate : public NotificationDelegate {
+ public:
+  DeleteOnCloseDelegate(MessageCenter* message_center,
+                        const std::string& notification_id)
+      : message_center_(message_center), notification_id_(notification_id) {}
+  DeleteOnCloseDelegate(const DeleteOnCloseDelegate&) = delete;
+  DeleteOnCloseDelegate& operator=(const DeleteOnCloseDelegate&) = delete;
+
+  void Close(bool by_user) override {
+    // Removing the same notification inside Close should be a noop.
+    message_center_->RemoveNotification(notification_id_, false /* by_user */);
+  }
+  void Click(const base::Optional<int>& button_index,
+             const base::Optional<base::string16>& reply) override {}
+
+ private:
+  ~DeleteOnCloseDelegate() override = default;
+
+  MessageCenter* message_center_;
+  std::string notification_id_;
+};
+
 // The default app id used to create simple notifications.
 const std::string kDefaultAppId = "app1";
 
@@ -912,6 +934,24 @@ TEST_F(MessageCenterImplTest, RemoveNonVisibleNotification) {
   EXPECT_EQ(0u, message_center()->GetVisibleNotifications().size());
 }
 
+TEST_F(MessageCenterImplTest, RemoveInCloseHandler) {
+  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));
+  message_center()->AddNotification(std::move(notification));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id));
+
+  // Then remove the notification which calls RemoveNotification() reentrantly.
+  message_center()->RemoveNotification(id, true /* by_user */);
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
+}
+
 TEST_F(MessageCenterImplTest, FindNotificationsByAppId) {
   message_center()->SetHasMessageCenterView(true);