Add a queue to defer requests during itarating notification in MessageCenterImpl

A crash might happen when an observer to MessageCenter removes notification in handler, because future observer couldn't access to the notification. This patch fixes the issue by adding a queue and defering requests during itaration.

Bug: 762803
Test: Added test passes
Change-Id: Id2c9a34602e14869c9b6db7d6dd7cff94b0089f0
Reviewed-on: https://chromium-review.googlesource.com/676624Reviewed-by: Eliot Courtney <edcourtney@chromium.org>
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508609}

ui/message_center/BUILD.gn

@@ -57,6 +57,8 @@ component("message_center") {
       "//build/config/compiler:no_size_t_to_int_warning",
     ]
     sources = [
+      "change_queue.cc",
+      "change_queue.h",
       "cocoa/notification_controller.h",
       "cocoa/notification_controller.mm",
       "cocoa/opaque_views.h",
@@ -205,6 +207,7 @@ if (enable_message_center) {
 
   test("message_center_unittests") {
     sources = [
+      "change_queue_unittest.cc",
       "cocoa/notification_controller_unittest.mm",
       "cocoa/popup_collection_unittest.mm",
       "cocoa/popup_controller_unittest.mm",

ui/message_center/change_queue.cc

+// Copyright 2017 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 "ui/message_center/change_queue.h"
+
+#include "ui/message_center/message_center_impl.h"
+#include "ui/message_center/notification.h"
+
+namespace message_center {
+
+// Change represents an operation made on a notification. Since it contains
+// the final state of the notification, except complex cases, we generally
+// optimize the list and keep only the last change for a particular notification
+// that is in the notification list around. There are two ids; |id_| is the
+// post-update notification id that has been assigned by an update, and
+// |previous_id| is the previous id of the notification before the Change.
+// The two ids are same unless the Change changes the id of the notification.
+// See the comments of id() and previous_id() for reference.
+class ChangeQueue::Change {
+ public:
+  Change(ChangeType type,
+         const std::string& id,
+         std::unique_ptr<Notification> notification);
+  ~Change();
+
+  // Used to transfer ownership of the contained notification.
+  std::unique_ptr<Notification> PassNotification();
+
+  Notification* notification() const { return notification_.get(); }
+  // Returns the post-update ID. It means:
+  // - ADD event: ID of the notification to be added. In this case, this must be
+  //   same as |previous_id()|.
+  // - UPDATE event: ID of the notification after the change. If the change
+  //   doesn't update its ID, this value is same as |previous_id()|.
+  // - DELETE event: ID of the notification to be deleted. This must be same as
+  //   |previous_id()|.
+  const std::string& id() const { return id_; }
+  ChangeType type() const { return type_; }
+  bool by_user() const { return by_user_; }
+  void set_by_user(bool by_user) { by_user_ = by_user; }
+  // Returns the ID which is used in the notification list. In other word, it
+  // means the ID before the change.
+  const std::string& previous_id() const { return previous_id_; }
+  void set_type(const ChangeType new_type) { type_ = new_type; }
+  void ReplaceNotification(std::unique_ptr<Notification> new_notification);
+
+ private:
+  ChangeType type_;
+  std::string id_;
+  std::string previous_id_;
+  bool by_user_;
+  std::unique_ptr<Notification> notification_;
+
+  DISALLOW_COPY_AND_ASSIGN(Change);
+};
+
+////////////////////////////////////////////////////////////////////////////////
+// ChangeFinder
+
+struct ChangeFinder {
+  explicit ChangeFinder(const std::string& id) : id(id) {}
+  bool operator()(const std::unique_ptr<ChangeQueue::Change>& change) {
+    return change->id() == id;
+  }
+
+  std::string id;
+};
+
+////////////////////////////////////////////////////////////////////////////////
+// ChangeQueue::Change
+
+ChangeQueue::Change::Change(ChangeType type,
+                            const std::string& id,
+                            std::unique_ptr<Notification> notification)
+    : type_(type),
+      previous_id_(id),
+      by_user_(false),
+      notification_(std::move(notification)) {
+  DCHECK(!id.empty());
+  DCHECK(type != CHANGE_TYPE_DELETE || !notification_);
+
+  id_ = notification_ ? notification_->id() : previous_id_;
+}
+
+ChangeQueue::Change::~Change() {}
+
+std::unique_ptr<Notification> ChangeQueue::Change::PassNotification() {
+  return std::move(notification_);
+}
+
+void ChangeQueue::Change::ReplaceNotification(
+    std::unique_ptr<Notification> new_notification) {
+  id_ = new_notification ? new_notification->id() : previous_id_;
+  notification_.swap(new_notification);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// ChangeQueue
+
+ChangeQueue::ChangeQueue() = default;
+
+ChangeQueue::~ChangeQueue() = default;
+
+void ChangeQueue::ApplyChanges(MessageCenterImpl* message_center) {
+  while (!changes_.empty()) {
+    auto iter = changes_.begin();
+    std::unique_ptr<Change> change(std::move(*iter));
+    changes_.erase(iter);
+    ApplyChangeInternal(message_center, std::move(change));
+  }
+}
+
+void ChangeQueue::AddNotification(std::unique_ptr<Notification> notification) {
+  std::string id = notification->id();
+  changes_.push_back(
+      std::make_unique<Change>(CHANGE_TYPE_ADD, id, std::move(notification)));
+}
+
+void ChangeQueue::UpdateNotification(
+    const std::string& old_id,
+    std::unique_ptr<Notification> notification) {
+  changes_.push_back(std::make_unique<Change>(CHANGE_TYPE_UPDATE, old_id,
+                                              std::move(notification)));
+}
+
+void ChangeQueue::RemoveNotification(const std::string& id, bool by_user) {
+  auto change = std::make_unique<Change>(CHANGE_TYPE_DELETE, id, nullptr);
+  change->set_by_user(by_user);
+  changes_.push_back(std::move(change));
+}
+
+Notification* ChangeQueue::GetLatestNotification(const std::string& id) const {
+  auto iter =
+      std::find_if(changes_.rbegin(), changes_.rend(), ChangeFinder(id));
+  if (iter == changes_.rend())
+    return nullptr;
+
+  if ((*iter)->type() == CHANGE_TYPE_DELETE)
+    return nullptr;
+
+  return (*iter)->notification();
+}
+
+void ChangeQueue::ApplyChangeInternal(MessageCenterImpl* message_center,
+                                      std::unique_ptr<Change> change) {
+  switch (change->type()) {
+    case CHANGE_TYPE_ADD:
+      message_center->AddNotificationImmediately(change->PassNotification());
+      break;
+    case CHANGE_TYPE_UPDATE:
+      message_center->UpdateNotificationImmediately(change->previous_id(),
+                                                    change->PassNotification());
+      break;
+    case CHANGE_TYPE_DELETE:
+      message_center->RemoveNotificationImmediately(change->previous_id(),
+                                                    change->by_user());
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
+}  // namespace message_center

ui/message_center/change_queue.h

+// Copyright 2017 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.
+
+#ifndef UI_MESSAGE_CENTER_CHANGE_QUEUE_H_
+#define UI_MESSAGE_CENTER_CHANGE_QUEUE_H_
+
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "ui/message_center/message_center_export.h"
+
+namespace message_center {
+
+class MessageCenterImpl;
+class Notification;
+
+// ChangeQueue keeps track of all the changes. This class can be used for
+// queuing changes during the message center can't be updated.
+class MESSAGE_CENTER_EXPORT ChangeQueue {
+ public:
+  enum ChangeType {
+    CHANGE_TYPE_ADD = 0,
+    CHANGE_TYPE_UPDATE,
+    CHANGE_TYPE_DELETE
+  };
+
+  // Change represents an operation made on a notification.  Since it contains
+  // the final state of the notification, we only keep the last change for a
+  // particular notification that is in the notification list around.  There are
+  // two ids; |id_| is the newest notification id that has been assigned by an
+  // update, and |notification_list_id_| is the id of the notification it should
+  // be updating as it exists in the notification list.
+  class Change;
+
+  ChangeQueue();
+  ~ChangeQueue();
+
+  // Called when the message center has appropriate visibility.  Modifies
+  // |message_center| but does not retain it.  This also causes the queue to
+  // empty itself.
+  void ApplyChanges(MessageCenterImpl* message_center);
+
+  // Causes a TYPE_ADD change to be added to the queue.
+  void AddNotification(std::unique_ptr<Notification> notification);
+
+  // Causes a TYPE_UPDATE change to be added to the queue.
+  void UpdateNotification(const std::string& old_id,
+                          std::unique_ptr<Notification> notification);
+
+  // Causes a TYPE_DELETE change to be added to the queue.
+  void RemoveNotification(const std::string& id, bool by_user);
+
+  // Returns the pointer to the Notification in the latest Change which has the
+  // given ID. If the Change is UPDATE, the ID after the change is searched.
+  // If the Change id REMOVE, this returns nullptr.
+  // ChangeQueue keeps retaining ownership of the Notification. The returned
+  // notification can be modified or be copied by caller.
+  Notification* GetLatestNotification(const std::string& id) const;
+
+ private:
+  void ApplyChangeInternal(MessageCenterImpl* message_center,
+                           std::unique_ptr<Change> change);
+
+  std::vector<std::unique_ptr<Change>> changes_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChangeQueue);
+};
+
+}  // namespace message_center
+
+#endif  // UI_MESSAGE_CENTER_CHANGE_QUEUE_H_

ui/message_center/change_queue_unittest.cc

+// Copyright 2017 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 "ui/message_center/change_queue.h"
+
+#include <utility>
+
+#include "base/strings/utf_string_conversions.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/message_center/message_center_impl.h"
+
+using base::UTF8ToUTF16;
+
+namespace message_center {
+
+namespace {
+
+Notification* CreateSimpleNotification(const std::string& id) {
+  RichNotificationData optional_fields;
+  optional_fields.buttons.push_back(ButtonInfo(UTF8ToUTF16("foo")));
+  optional_fields.buttons.push_back(ButtonInfo(UTF8ToUTF16("foo")));
+  return new Notification(
+      NOTIFICATION_TYPE_SIMPLE, id, UTF8ToUTF16("title"), UTF8ToUTF16(id),
+      gfx::Image() /* icon */, base::string16() /* display_source */, GURL(),
+      NotifierId(NotifierId::APPLICATION, "change_queue_test"), optional_fields,
+      NULL);
+}
+
+}  // namespace
+
+class ChangeQueueTest : public testing::Test {
+ public:
+  ChangeQueueTest() = default;
+  ~ChangeQueueTest() override = default;
+
+  MessageCenterImpl* message_center() { return &message_center_; }
+
+ private:
+  MessageCenterImpl message_center_;
+};
+
+TEST_F(ChangeQueueTest, Queueing) {
+  ChangeQueue queue;
+  std::string id("id1");
+  std::string id2("id2");
+  NotifierId notifier_id1(NotifierId::APPLICATION, "app1");
+
+  // First, add and update a notification to ensure updates happen
+  // normally.
+  std::unique_ptr<Notification> notification(CreateSimpleNotification(id));
+  message_center()->AddNotification(std::move(notification));
+  notification.reset(CreateSimpleNotification(id2));
+  message_center()->UpdateNotification(id, std::move(notification));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id2));
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
+
+  // Then update a notification; nothing should have happened.
+  notification.reset(CreateSimpleNotification(id));
+  queue.UpdateNotification(id2, std::move(notification));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id2));
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
+
+  // Apply the changes from the queue.
+  queue.ApplyChanges(message_center());
+
+  // Close the message center; then the update should have propagated.
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id2));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id));
+}
+
+TEST_F(ChangeQueueTest, SimpleQueueing) {
+  ChangeQueue queue;
+  std::string ids[6] = {"0", "1", "2", "3", "4", "5"};
+  NotifierId notifier_id1(NotifierId::APPLICATION, "app1");
+
+  std::unique_ptr<Notification> notification;
+  // Prepare initial notifications on NotificationList.
+  // NL: ["0", "1", "2", "4"]
+  int i = 0;
+  for (; i < 3; i++) {
+    notification.reset(CreateSimpleNotification(ids[i]));
+    message_center()->AddNotification(std::move(notification));
+  }
+  for (i = 0; i < 3; i++) {
+    EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[i]));
+  }
+  for (; i < 6; i++) {
+    EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[i]));
+  }
+
+  // This should update notification "1" to have id "4".
+  notification.reset(CreateSimpleNotification(ids[4]));
+  queue.AddNotification(std::move(notification));
+
+  // Change the ID: "2" -> "5", "5" -> "3".
+  notification.reset(CreateSimpleNotification(ids[5]));
+  queue.UpdateNotification(ids[2], std::move(notification));
+  notification.reset(CreateSimpleNotification(ids[3]));
+  queue.UpdateNotification(ids[5], std::move(notification));
+
+  // This should update notification "4" to "5".
+  notification.reset(CreateSimpleNotification(ids[5]));
+  queue.UpdateNotification(ids[4], std::move(notification));
+
+  // This should create a new "4", that doesn't overwrite the update from 4
+  // before.
+  notification.reset(CreateSimpleNotification(ids[4]));
+  queue.AddNotification(std::move(notification));
+
+  // The NL should still be the same: ["0", "1", "2", "4"]
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[0]));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[1]));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[2]));
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[3]));
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[4]));
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[5]));
+  EXPECT_EQ(message_center()->GetVisibleNotifications().size(), 3u);
+
+  // Apply the changes from the queue.
+  queue.ApplyChanges(message_center());
+
+  // Changes should be applied.
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[0]));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[1]));
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(ids[2]));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[3]));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[4]));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(ids[5]));
+  EXPECT_EQ(message_center()->GetVisibleNotifications().size(), 5u);
+}
+
+}  // namespace message_center

ui/message_center/message_center_impl.h

@@ -16,6 +16,7 @@
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "ui/message_center/change_queue.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/message_center_observer.h"
 #include "ui/message_center/message_center_types.h"
@@ -25,8 +26,13 @@
 
 namespace message_center {
 
+namespace internal {
+class ScopedNotificationsIterationLock;
+};
+
 // The default implementation of MessageCenter.
-class MessageCenterImpl : public MessageCenter,
+class MESSAGE_CENTER_EXPORT MessageCenterImpl
+    : public MessageCenter,
       public NotificationBlocker::Observer,
       public message_center::NotifierSettingsObserver {
  public:
@@ -102,6 +108,7 @@ class MessageCenterImpl : public MessageCenter,
   void DisableTimersForTest() override;
 
  private:
+  friend internal::ScopedNotificationsIterationLock;
   struct NotificationCache {
     NotificationCache();
     ~NotificationCache();
@@ -111,7 +118,10 @@ class MessageCenterImpl : public MessageCenter,
     NotificationList::Notifications visible_notifications;
     size_t unread_count;
   };
+  class ScopedNotificationsLock;
 
+  Notification* GetLatestNotificationIncludingQueued(
+      const std::string& id) const;
   void RemoveNotificationsForNotifierId(const NotifierId& notifier_id);
 
   THREAD_CHECKER(thread_checker_);
@@ -123,10 +133,14 @@ class MessageCenterImpl : public MessageCenter,
   std::unique_ptr<base::OneShotTimer> quiet_mode_timer_;
   NotifierSettingsProvider* settings_provider_;
   std::vector<NotificationBlocker*> blockers_;
+  std::unique_ptr<ChangeQueue> notification_change_queue_;
 
   bool locked_ = false;
   bool visible_ = false;
 
+  // modified by ScopedNotificationsIterationLock.
+  bool iterating_ = false;
+
   base::string16 product_os_name_;
 
   DISALLOW_COPY_AND_ASSIGN(MessageCenterImpl);

ui/message_center/message_center_impl_unittest.cc

@@ -31,6 +31,48 @@ using base::UTF8ToUTF16;
 
 namespace message_center {
 
+namespace {
+
+class CheckObserver : public MessageCenterObserver {
+ public:
+  CheckObserver(MessageCenter* message_center, const std::string& target_id)
+      : message_center_(message_center), target_id_(target_id) {
+    DCHECK(message_center);
+    DCHECK(!target_id.empty());
+  }
+
+  void OnNotificationUpdated(const std::string& notification_id) override {
+    EXPECT_TRUE(message_center_->FindVisibleNotificationById(target_id_));
+  }
+
+ private:
+  MessageCenter* message_center_;
+  std::string target_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(CheckObserver);
+};
+
+class RemoveObserver : public MessageCenterObserver {
+ public:
+  RemoveObserver(MessageCenter* message_center, const std::string& target_id)
+      : message_center_(message_center), target_id_(target_id) {
+    DCHECK(message_center);
+    DCHECK(!target_id.empty());
+  }
+
+  void OnNotificationUpdated(const std::string& notification_id) override {
+    message_center_->RemoveNotification(target_id_, false);
+  }
+
+ private:
+  MessageCenter* message_center_;
+  std::string target_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(RemoveObserver);
+};
+
+}  // anonymous namespace
+
 class MessageCenterImplTest : public testing::Test,
                               public MessageCenterObserver {
  public:
@@ -768,5 +810,36 @@ TEST_F(MessageCenterImplTest, RemoveWhileMessageCenterVisible) {
   EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
 }
 
+TEST_F(MessageCenterImplTest, RemoveWhileIteratingObserver) {
+  std::string id("id1");
+  CheckObserver check1(message_center(), id);
+  CheckObserver check2(message_center(), id);
+  RemoveObserver remove(message_center(), id);
+
+  // Prepare a notification
+  std::unique_ptr<Notification> notification(CreateSimpleNotification(id));
+  message_center()->AddNotification(std::move(notification));
+  EXPECT_TRUE(message_center()->FindVisibleNotificationById(id));
+
+  // Install the test handlers
+  message_center()->AddObserver(&check1);
+  message_center()->AddObserver(&remove);
+  message_center()->AddObserver(&check2);
+
+  // Update the notification. The notification will be removed in the observer,
+  // but the actual removal will be done at the end of the iteration.
+  // Notification keeps alive during iteration. This is checked by
+  // CheckObserver.
+  notification.reset(CreateSimpleNotification(id));
+  message_center()->UpdateNotification(id, std::move(notification));
+
+  // Notification is removed correctly.
+  EXPECT_FALSE(message_center()->FindVisibleNotificationById(id));
+
+  message_center()->RemoveObserver(&check1);
+  message_center()->RemoveObserver(&remove);
+  message_center()->RemoveObserver(&check2);
+}
+
 }  // namespace internal
 }  // namespace message_center