[Notifications] Fix crash in AddNotification.

ui/message_center/views/message_center_view.cc

Index: ui/message_center/views/message_center_view.cc
diff --git a/ui/message_center/views/message_center_view.cc b/ui/message_center/views/message_center_view.cc
index 05db825efbcc0b17a0a7ff65d1165fc37a1c5907..da980b136e8f2880039d15fd197505fd13cf2217 100644
--- a/ui/message_center/views/message_center_view.cc
+++ b/ui/message_center/views/message_center_view.cc
@@ -407,6 +407,8 @@ bool MessageCenterView::SetRepositionTarget() {
 }
 
 void MessageCenterView::OnNotificationUpdated(const std::string& id) {
+  // TODO(edcourtney): We may be able to remove this, since |UpdateNotification|
+  // checks it anyway.

[yoshiki, 2017/04/25 04:58:00]

How about adding NOTREACHED() before return?

[Eliot Courtney, 2017/04/25 05:06:52]

This can still be reached, but it's possible we don't need to check it since
UpdateNotification checks it anyway. We can't remove the check from
UpdateNotification because ResetRepositionSession can trigger notifications to
be removed, which seems like it will change notification_views_ (meaning that
the original order of checking this before ResetRepositionSession had a bug?).

[Eliot Courtney, 2017/04/25 06:29:12]

Discussed offline - added a TODO below to investigate if these checks are really
needed.

   NotificationViewsMap::const_iterator view_iter = notification_views_.find(id);
   if (view_iter == notification_views_.end())
     return;
@@ -416,30 +418,7 @@ void MessageCenterView::OnNotificationUpdated(const std::string& id) {
   if (!SetRepositionTarget())
     message_list_view_->ResetRepositionSession();
 
-  // TODO(dimich): add MessageCenter::GetVisibleNotificationById(id)
-  MessageView* view = view_iter->second;
-  const NotificationList::Notifications& notifications =
-      message_center_->GetVisibleNotifications();
-  for (NotificationList::Notifications::const_iterator iter =
-           notifications.begin(); iter != notifications.end(); ++iter) {
-    if ((*iter)->id() == id) {
-      int old_width = view->width();
-      int old_height = view->height();
-      bool old_pinned = view->IsPinned();
-      message_list_view_->UpdateNotification(view, **iter);
-      if (view->GetHeightForWidth(old_width) != old_height) {
-        Update(true /* animate */);
-      } else if (view->IsPinned() != old_pinned) {
-        // Animate flag is false, since the pinned flag transition doesn't need
-        // animation.
-        Update(false /* animate */);
-      }
-      break;
-    }
-  }
-
-  // Notify accessibility that the contents have changed.
-  view->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false);
+  UpdateNotification(id);
 }
 
 void MessageCenterView::OnLockedStateChanged(bool locked) {
@@ -481,7 +460,14 @@ void MessageCenterView::ClickOnSettingsButton(
 
 void MessageCenterView::UpdateNotificationSize(
     const std::string& notification_id) {
-  OnNotificationUpdated(notification_id);
+  // TODO(edcourtney, yoshiki): We don't call OnNotificationUpdated directly
+  // because it resets the reposition session, which can end up deleting
+  // notification items when it cancels animations. This causes problems for
+  // ARC notifications. See crbug.com/714493. OnNotificationUpdated should not
+  // have to consider the reposition session, but OnMouseEntered and
+  // OnMouseExited don't work properly for ARC notifications at the moment.
+  // See crbug.com/714587.
+  UpdateNotification(notification_id);
 }
 
 void MessageCenterView::AnimationEnded(const gfx::Animation* animation) {
@@ -673,4 +659,36 @@ void MessageCenterView::SetNotificationViewForTest(MessageView* view) {
   message_list_view_->AddNotificationAt(view, 0);
 }
 
+void MessageCenterView::UpdateNotification(const std::string& id) {
+  NotificationViewsMap::const_iterator view_iter = notification_views_.find(id);
+  if (view_iter == notification_views_.end())
+    return;

[yoshiki, 2017/04/25 04:58:01]

This should also not be happened. How about adding NOTREACHED()?

[Eliot Courtney, 2017/04/25 05:06:52]

I believe this can happen when ResetRepositionSession triggers notifications to
be removed - OnNotificationRemoved in this class may be called. The flow is:

ResetRepositionSession => BoundsAnimator::Cancel =>
MessageListView::OnBoundsAnimatorDone => OnAllNotificationsCleared =>
MessageCenterView::OnAllNotificationsCleared =>
MessageCenterImpl::RemoveAllNotifications => NotificationDelegate::Close() =>
OnNotificationRemoved

[Eliot Courtney, 2017/04/25 06:29:12]

I investigated a bit, and it seems that when close all is pressed on the message
center, it unregisters itself as an observer on MessageCenterImpl, so it never
gets the OnNotificationRemoved calls that would cause this statement to fire.
Discussed offline, and I've added a TODO to investigate whether or not we can
remove this check.

+
+  // TODO(dimich): add MessageCenter::GetVisibleNotificationById(id)
+  MessageView* view = view_iter->second;
+  const NotificationList::Notifications& notifications =
+      message_center_->GetVisibleNotifications();
+  for (NotificationList::Notifications::const_iterator iter =
+           notifications.begin();
+       iter != notifications.end(); ++iter) {
+    if ((*iter)->id() == id) {
+      int old_width = view->width();
+      int old_height = view->height();
+      bool old_pinned = view->IsPinned();
+      message_list_view_->UpdateNotification(view, **iter);
+      if (view->GetHeightForWidth(old_width) != old_height) {
+        Update(true /* animate */);
+      } else if (view->IsPinned() != old_pinned) {
+        // Animate flag is false, since the pinned flag transition doesn't need
+        // animation.
+        Update(false /* animate */);
+      }
+      break;
+    }
+  }
+
+  // Notify accessibility that the contents have changed.
+  view->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false);
+}
+
 }  // namespace message_center