Notifications: Delay closing out of workarea bounds pop-ups.

When the pop-up is expanded to show inline settings, the new height can overflow
the work area. Deleting the pop-up in this scenario triggers a crash as we're
still in the same call stack.

BUG=957033

Change-Id: Ie8d4db393893b662b72f9c04317f9ca640a25607
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1600155
Commit-Queue: Tim Song <tengs@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658793}

ui/message_center/views/message_popup_collection.cc

@@ -4,6 +4,7 @@
 
 #include "ui/message_center/views/message_popup_collection.h"
 
+#include "base/bind.h"
 #include "base/stl_util.h"
 #include "ui/gfx/animation/linear_animation.h"
 #include "ui/gfx/animation/tween.h"
@@ -29,7 +30,8 @@ constexpr base::TimeDelta kMoveDownDuration =
 MessagePopupCollection::MessagePopupCollection(
     PopupAlignmentDelegate* alignment_delegate)
     : animation_(std::make_unique<gfx::LinearAnimation>(this)),
-      alignment_delegate_(alignment_delegate) {
+      alignment_delegate_(alignment_delegate),
+      weak_ptr_factory_(this) {
   MessageCenter::Get()->AddObserver(this);
   alignment_delegate_->set_collection(this);
 }
@@ -294,7 +296,14 @@ void MessagePopupCollection::TransitionToAnimation() {
     resize_requested_ = false;
     state_ = State::MOVE_DOWN;
     MoveDownPopups();
-    ClosePopupsOutsideWorkArea();
+
+    // This function may be called by a child MessageView when a notification is
+    // expanded by the user.  Deleting the pop-up should be delayed so we are
+    // out of the child view's call stack. See crbug.com/957033.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&MessagePopupCollection::ClosePopupsOutsideWorkArea,
+                       weak_ptr_factory_.GetWeakPtr()));
     return;
   }
 

ui/message_center/views/message_popup_collection.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/memory/weak_ptr.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/message_center/message_center_export.h"
@@ -237,6 +238,8 @@ class MESSAGE_CENTER_EXPORT MessagePopupCollection
   //   * a notification comes out: FADE_OUT
   bool inverse_ = false;
 
+  base::WeakPtrFactory<MessagePopupCollection> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(MessagePopupCollection);
 };
 

ui/message_center/views/message_popup_collection_unittest.cc

@@ -268,18 +268,22 @@ class MessagePopupCollectionTest : public views::ViewsTestBase,
   bool IsAnimating() const { return popup_collection_->IsAnimating(); }
 
   void AnimateUntilIdle() {
-    while (popup_collection_->IsAnimating())
+    while (popup_collection_->IsAnimating()) {
       popup_collection_->SetAnimationValue(1.0);
+      RunPendingMessages();
+    }
   }
 
   void AnimateToMiddle() {
     EXPECT_TRUE(popup_collection_->IsAnimating());
     popup_collection_->SetAnimationValue(0.5);
+    RunPendingMessages();
   }
 
   void AnimateToEnd() {
     EXPECT_TRUE(popup_collection_->IsAnimating());
     popup_collection_->SetAnimationValue(1.0);
+    RunPendingMessages();
   }
 
   MockMessagePopupView* GetPopup(const std::string& id) {
@@ -457,6 +461,7 @@ TEST_F(MessagePopupCollectionTest, UpdateContentsCausesPopupClose) {
   auto updated_notification = CreateNotification(id);
   updated_notification->set_message(base::ASCIIToUTF16("updated"));
   MessageCenter::Get()->UpdateNotification(id, std::move(updated_notification));
+  RunPendingMessages();
   EXPECT_EQ(0u, GetPopupCounts());
 }
 
@@ -838,6 +843,7 @@ TEST_F(MessagePopupCollectionTest, PopupResizedAndOverflown) {
   GetPopup(id1)->SetPreferredHeight(changed_height);
 
   AnimateUntilIdle();
+  RunPendingMessages();
 
   EXPECT_TRUE(GetPopup(id0));
   EXPECT_TRUE(work_area().Contains(GetPopup(id0)->GetBoundsInScreen()));

ui/message_center/views/notification_view_md.cc

@@ -715,12 +715,7 @@ void NotificationViewMD::ButtonPressed(views::Button* sender,
   if (sender == header_row_) {
     if (IsExpandable() && content_row_->visible()) {
       SetManuallyExpandedOrCollapsed(true);
-      auto weak_ptr = weak_ptr_factory_.GetWeakPtr();
       ToggleExpanded();
-      // Check |this| is valid before continuing, because ToggleExpanded() might
-      // cause |this| to be deleted.
-      if (!weak_ptr)
-        return;
       Layout();
       SchedulePaint();
     }
@@ -1242,16 +1237,7 @@ void NotificationViewMD::ToggleInlineSettings(const ui::Event& event) {
   dont_block_button_->SetChecked(true);
 
   SetSettingMode(inline_settings_visible);
-
-  // Grab a weak pointer before calling SetExpanded() as it might cause |this|
-  // to be deleted.
-  {
-    auto weak_ptr = weak_ptr_factory_.GetWeakPtr();
-    SetExpanded(!inline_settings_visible);
-    if (!weak_ptr)
-      return;
-  }
-
+  SetExpanded(!inline_settings_visible);
   PreferredSizeChanged();
 
   if (inline_settings_visible)

ui/message_center/views/notification_view_md.h

@@ -325,8 +325,6 @@ class MESSAGE_CENTER_EXPORT NotificationViewMD
 
   base::TimeTicks last_mouse_pressed_timestamp_;
 
-  base::WeakPtrFactory<NotificationViewMD> weak_ptr_factory_{this};
-
   DISALLOW_COPY_AND_ASSIGN(NotificationViewMD);
 };
 

ui/message_center/views/notification_view_md_unittest.cc

@@ -139,11 +139,6 @@ class NotificationViewMDTest
   void InkDropAnimationStarted() override;
   void InkDropRippleAnimationEnded(views::InkDropState ink_drop_state) override;
 
-  void set_delete_on_preferred_size_changed(
-      bool delete_on_preferred_size_changed) {
-    delete_on_preferred_size_changed_ = delete_on_preferred_size_changed;
-  }
-
   void set_delete_on_notification_removed(bool delete_on_notification_removed) {
     delete_on_notification_removed_ = delete_on_notification_removed;
   }
@@ -170,7 +165,6 @@ class NotificationViewMDTest
   views::View* GetCloseButton();
 
   bool ink_drop_stopped_ = false;
-  bool delete_on_preferred_size_changed_ = false;
   bool delete_on_notification_removed_ = false;
   std::set<std::string> removed_ids_;
   scoped_refptr<NotificationTestDelegate> delegate_;
@@ -217,8 +211,7 @@ void NotificationViewMDTest::SetUp() {
 void NotificationViewMDTest::TearDown() {
   MessageCenter::Get()->RemoveObserver(this);
 
-  DCHECK(notification_view_ || delete_on_preferred_size_changed_ ||
-         delete_on_notification_removed_);
+  DCHECK(notification_view_ || delete_on_notification_removed_);
   if (notification_view_) {
     notification_view_->SetInkDropMode(MessageView::InkDropMode::OFF);
     notification_view_->RemoveObserver(this);
@@ -232,11 +225,6 @@ void NotificationViewMDTest::TearDown() {
 void NotificationViewMDTest::OnViewPreferredSizeChanged(
     views::View* observed_view) {
   EXPECT_EQ(observed_view, notification_view());
-  if (delete_on_preferred_size_changed_) {
-    widget()->CloseNow();
-    notification_view_.reset();
-    return;
-  }
   widget()->SetSize(notification_view()->GetPreferredSize());
 }
 
@@ -1210,23 +1198,6 @@ TEST_F(NotificationViewMDTest, TestClickExpanded) {
   EXPECT_TRUE(delegate_->clicked());
 }
 
-TEST_F(NotificationViewMDTest, TestDeleteOnToggleExpanded) {
-  std::unique_ptr<Notification> notification = CreateSimpleNotification();
-  notification->set_type(NotificationType::NOTIFICATION_TYPE_SIMPLE);
-  notification->set_title(base::string16());
-  notification->set_message(base::ASCIIToUTF16(
-      "consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore "
-      "et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud "
-      "exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat."));
-  UpdateNotificationViews(*notification);
-  EXPECT_FALSE(notification_view()->expanded_);
-
-  // The view can be deleted by PreferredSizeChanged(). https://crbug.com/918933
-  set_delete_on_preferred_size_changed(true);
-  notification_view()->ButtonPressed(notification_view()->header_row_,
-                                     DummyEvent());
-}
-
 TEST_F(NotificationViewMDTest, TestDeleteOnDisableNotification) {
   std::unique_ptr<Notification> notification = CreateSimpleNotification();
   notification->set_type(NOTIFICATION_TYPE_SIMPLE);