Fix MessagePopupCollection crash on small screen.

This CL fixes a reentrancy issue in MessagePopupCollection that might
cause crash on devices with small screen height.
We should set |is_updating_| to true when we call MessagePopupView::
UpdateContents(). Updating contents can change its height, and it might
cause the popup to be dismissed because the screen height might not be
enough to show the notification. The popup should be closed after the
update is finished, but we didn't set |is_updating_| to true, so the
popup could accidentally close during UpdateContents().

We didn't catch this until a11y related change
https://crrev.com/c/1166231 because |message_view_| was not used after
NotifyPopupResized() in MessagePopupView::UpdateContents().

TEST=MessagePopupCollection.UpdateContentsCausesPopupClose
BUG=874777

Change-Id: I29cfffec7e967254da0d880667e38805d0cded5a
Reviewed-on: https://chromium-review.googlesource.com/1179506
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584362}

ui/message_center/views/message_popup_collection.cc

@@ -141,7 +141,8 @@ void MessagePopupCollection::OnNotificationRemoved(
 
 void MessagePopupCollection::OnNotificationUpdated(
     const std::string& notification_id) {
-  RemoveClosedPopupItems();
+  if (is_updating_)
+    return;
 
   // Find Notification object with |notification_id|.
   const auto& notifications = MessageCenter::Get()->GetPopupNotifications();
@@ -159,11 +160,17 @@ void MessagePopupCollection::OnNotificationUpdated(
     return;
   }
 
-  // Update contents of the notification.
-  for (auto& item : popup_items_) {
-    if (item.id == notification_id) {
-      item.popup->UpdateContents(**it);
-      break;
+  {
+    base::AutoReset<bool> reset(&is_updating_, true);
+
+    RemoveClosedPopupItems();
+
+    // Update contents of the notification.
+    for (const auto& item : popup_items_) {
+      if (item.id == notification_id) {
+        item.popup->UpdateContents(**it);
+        break;
+      }
     }
   }
 

ui/message_center/views/message_popup_collection_unittest.cc

@@ -130,6 +130,9 @@ class MockMessagePopupView : public MessagePopupView {
   }
 
   void UpdateContents(const Notification& notification) override {
+    if (height_after_update_.has_value())
+      SetPreferredHeight(height_after_update_.value());
+    popup_collection_->NotifyPopupResized();
     updated_ = true;
   }
 
@@ -164,12 +167,18 @@ class MockMessagePopupView : public MessagePopupView {
 
   void set_expandable(bool expandable) { expandable_ = expandable; }
 
+  void set_height_after_update(base::Optional<int> height_after_update) {
+    height_after_update_ = height_after_update;
+  }
+
  private:
   MockMessagePopupCollection* const popup_collection_;
 
   std::string id_;
   bool updated_ = false;
   bool expandable_ = false;
+
+  base::Optional<int> height_after_update_;
 };
 
 MessagePopupView* MockMessagePopupCollection::CreatePopup(
@@ -382,6 +391,21 @@ TEST_F(MessagePopupCollectionTest, UpdateContents) {
   EXPECT_TRUE(GetPopup(id)->updated());
 }
 
+TEST_F(MessagePopupCollectionTest, UpdateContentsCausesPopupClose) {
+  std::string id = AddNotification();
+  AnimateToEnd();
+  EXPECT_FALSE(IsAnimating());
+  EXPECT_EQ(1u, GetPopupCounts());
+  EXPECT_FALSE(GetPopup(id)->updated());
+
+  GetPopup(id)->set_height_after_update(2048);
+
+  auto updated_notification = CreateNotification(id);
+  updated_notification->set_message(base::ASCIIToUTF16("updated"));
+  MessageCenter::Get()->UpdateNotification(id, std::move(updated_notification));
+  EXPECT_EQ(0u, GetPopupCounts());
+}
+
 TEST_F(MessagePopupCollectionTest, MarkAllPopupsShown) {
   for (size_t i = 0; i < kMaxVisiblePopupNotifications; ++i)
     AddNotification();

ui/message_center/views/message_popup_view.cc

@@ -67,6 +67,8 @@ MessagePopupView::~MessagePopupView() {
 }
 
 void MessagePopupView::UpdateContents(const Notification& notification) {
+  if (!IsWidgetValid())
+    return;
   ui::AXNodeData old_data;
   message_view_->GetAccessibleNodeData(&old_data);
   message_view_->UpdateWithNotification(notification);
@@ -82,26 +84,28 @@ void MessagePopupView::UpdateContents(const Notification& notification) {
 }
 
 float MessagePopupView::GetOpacity() const {
-  if (!GetWidget() || GetWidget()->IsClosed())
+  if (!IsWidgetValid())
     return 0.f;
   return GetWidget()->GetLayer()->opacity();
 }
 
 void MessagePopupView::SetPopupBounds(const gfx::Rect& bounds) {
-  if (!GetWidget() || GetWidget()->IsClosed())
+  if (!IsWidgetValid())
     return;
   GetWidget()->SetBounds(bounds);
 }
 
 void MessagePopupView::SetOpacity(float opacity) {
-  if (!GetWidget() || GetWidget()->IsClosed())
+  if (!IsWidgetValid())
     return;
   GetWidget()->SetOpacity(opacity);
 }
 
 void MessagePopupView::AutoCollapse() {
-  if (is_hovered_ || message_view_->IsManuallyExpandedOrCollapsed())
+  if (!IsWidgetValid() || is_hovered_ ||
+      message_view_->IsManuallyExpandedOrCollapsed()) {
     return;
+  }
   message_view_->SetExpanded(false);
 }
 
@@ -183,11 +187,10 @@ void MessagePopupView::OnDisplayChanged() {
 }
 
 void MessagePopupView::OnWorkAreaChanged() {
-  views::Widget* widget = GetWidget();
-  if (!widget || widget->IsClosed())
+  if (!IsWidgetValid())
     return;
 
-  gfx::NativeView native_view = widget->GetNativeView();
+  gfx::NativeView native_view = GetWidget()->GetNativeView();
   if (!native_view)
     return;
 
@@ -203,4 +206,8 @@ void MessagePopupView::OnWidgetActivationChanged(views::Widget* widget,
   popup_collection_->Update();
 }
 
+bool MessagePopupView::IsWidgetValid() const {
+  return GetWidget() && !GetWidget()->IsClosed();
+}
+
 }  // namespace message_center

ui/message_center/views/message_popup_view.h

@@ -71,6 +71,9 @@ class MESSAGE_CENTER_EXPORT MessagePopupView : public views::WidgetDelegateView,
                    MessagePopupCollection* popup_collection);
 
  private:
+  // True if the view has a widget and the widget is not closed.
+  bool IsWidgetValid() const;
+
   // Owned by views hierarchy.
   MessageView* message_view_;