[Notifications] Fix crash in AddNotification.

ui/message_center/views/message_center_view.cc

 // Copyright (c) 2013 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/views/message_center_view.h"
 
 #include <list>
 #include <map>
 
 #include "base/macros.h"
@@ skipping 389 matching lines @@
       if (hover_view->IsMouseHovered()) {
         message_list_view_->SetRepositionTarget(hover_view->bounds());
         return true;
       }
     }
   }
   return false;
 }
 
 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;
 
   // If there is no reposition target anymore, make sure to reset the reposition
   // session.
   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) {
   is_locked_ = locked;
   UpdateButtonBarStatus();
   Update(true /* animate */);
 }
 
 void MessageCenterView::ClickOnNotification(
     const std::string& notification_id) {
@@ skipping 21 matching lines @@
   message_center_->ClickOnNotificationButton(notification_id, button_index);
 }
 
 void MessageCenterView::ClickOnSettingsButton(
     const std::string& notification_id) {
   message_center_->ClickOnSettingsButton(notification_id);
 }
 
 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) {
   DCHECK_EQ(animation, settings_transition_animation_.get());
 
   Visibility visibility =
       mode_ == Mode::SETTINGS ? VISIBILITY_SETTINGS : VISIBILITY_MESSAGE_CENTER;
   message_center_->SetVisibility(visibility);
 
   if (source_view_) {
@@ skipping 171 matching lines @@
   } else {
     // Disable the close-all button since no notification is visible.
     button_bar_->SetCloseAllButtonEnabled(false);
   }
 }
 
 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