Fix use-after-free in message center.

MessageCenterImpl::OnBlockingStateChanged() was unsafe. On desktop,
MarkSinglePopupAsShown removes/deletes that notification since there is
no message center bubble. The function then references the notification
again, via the |blocked| list. Instead of a list of weak Notification
pointers, |blocked| should be a list of notification IDs.

This bug was revealed by (although not directly caused by) removing the
NotificationChangeQueue.

Bug: 816374
Change-Id: I9ebc21ae95b18bf167c52911c7c0e1eacaf6e06d
Reviewed-on: https://chromium-review.googlesource.com/938676Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539497}

ui/message_center/message_center_impl.cc

@@ -72,25 +72,25 @@ void MessageCenterImpl::RemoveNotificationBlocker(
 
 void MessageCenterImpl::OnBlockingStateChanged(NotificationBlocker* blocker) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  std::list<const Notification*> blocked;
+  std::list<std::string> blocked;
   NotificationList::PopupNotifications popups =
       notification_list_->GetPopupNotifications(blockers_, &blocked);
 
   // Close already displayed pop-ups that are blocked now.
-  for (const auto* notification : blocked) {
+  for (const std::string& notification_id : blocked) {
     // Do not call MessageCenterImpl::MarkSinglePopupAsShown() directly here
     // just for performance reason. MessageCenterImpl::MarkSinglePopupAsShown()
     // calls NotificationList::MarkSinglePopupAsShown(), but the whole cache
     // will be recreated below.
-    if (notification->IsRead())
-      notification_list_->MarkSinglePopupAsShown(notification->id(), true);
+    if (FindVisibleNotificationById(notification_id)->IsRead())
+      notification_list_->MarkSinglePopupAsShown(notification_id, true);
   }
   visible_notifications_ =
       notification_list_->GetVisibleNotifications(blockers_);
 
-  for (const auto* notification : blocked) {
+  for (const std::string& notification_id : blocked) {
     for (auto& observer : observer_list_)
-      observer.OnNotificationUpdated(notification->id());
+      observer.OnNotificationUpdated(notification_id);
   }
   for (auto& observer : observer_list_)
     observer.OnBlockingStateChanged(blocker);
@@ -150,7 +150,7 @@ MessageCenterImpl::GetVisibleNotifications() {
 NotificationList::PopupNotifications
     MessageCenterImpl::GetPopupNotifications() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  return notification_list_->GetPopupNotifications(blockers_, NULL);
+  return notification_list_->GetPopupNotifications(blockers_, nullptr);
 }
 
 //------------------------------------------------------------------------------

ui/message_center/notification_list.cc

@@ -177,9 +177,9 @@ bool NotificationList::HasPopupNotifications(
   return false;
 }
 
-NotificationList::PopupNotifications NotificationList::GetPopupNotifications(
-    const NotificationBlockers& blockers,
-    std::list<const Notification*>* blocked) {
+NotificationList::PopupNotifications
+NotificationList::GetPopupNotifications(const NotificationBlockers& blockers,
+                                        std::list<std::string>* blocked) {
   PopupNotifications result;
   size_t default_priority_popup_count = 0;
 
@@ -196,7 +196,7 @@ NotificationList::PopupNotifications NotificationList::GetPopupNotifications(
 
     if (!ShouldShowNotificationAsPopup(*notification, blockers)) {
       if (blocked)
-        blocked->push_back(notification);
+        blocked->push_back(notification->id());
       continue;
     }
 

ui/message_center/notification_list.h

@@ -113,9 +113,8 @@ class MESSAGE_CENTER_EXPORT NotificationList {
   // It also stores the list of notifications which are blocked by |blockers|
   // to |blocked|. |blocked| can be NULL if the caller doesn't care which
   // notifications are blocked.
-  PopupNotifications GetPopupNotifications(
-      const NotificationBlockers& blockers,
-      std::list<const Notification*>* blocked);
+  PopupNotifications GetPopupNotifications(const NotificationBlockers& blockers,
+                                           std::list<std::string>* blocked);
 
   // Marks a specific popup item as shown. Set |mark_notification_as_read| to
   // true in case marking the notification as read too.