Ash Tray: Fix notification swipe controls button handler

When a settings or snooze button handler of the MessageView deletes
|this| synchronously, we can't access any local fields anymore. This CL
fixes that by getting a weak ptr to check if |this| is still valid after
calling the MessageView.

Bug: None
Change-Id: I8153a4c56fe5b5c91eafd5f9d1cdbcbc3f58857a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2328858Reviewed-by: Tim Song <tengs@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793562}

ash/BUILD.gn

@@ -1944,6 +1944,7 @@ test("ash_unittests") {
     "system/message_center/ash_message_popup_collection_unittest.cc",
     "system/message_center/inactive_user_notification_blocker_unittest.cc",
     "system/message_center/message_center_ui_controller_unittest.cc",
+    "system/message_center/notification_swipe_control_view_unittest.cc",
     "system/message_center/notifier_settings_view_unittest.cc",
     "system/message_center/session_state_notification_blocker_unittest.cc",
     "system/message_center/unified_message_center_bubble_unittest.cc",

ash/system/message_center/notification_swipe_control_view.cc

@@ -185,17 +185,25 @@ const char* NotificationSwipeControlView::GetClassName() const {
 void NotificationSwipeControlView::ButtonPressed(views::Button* sender,
                                                  const ui::Event& event) {
   DCHECK(sender);
+  std::string notification_id = message_view_->notification_id();
+  auto weak_this = weak_factory_.GetWeakPtr();
+
   if (sender == settings_button_) {
     message_view_->OnSettingsButtonPressed(event);
-    metrics_utils::LogSettingsShown(message_view_->notification_id(),
+    metrics_utils::LogSettingsShown(notification_id,
                                     /*is_slide_controls=*/true,
                                     /*is_popup=*/false);
   } else if (sender == snooze_button_) {
     message_view_->OnSnoozeButtonPressed(event);
-    metrics_utils::LogSnoozed(message_view_->notification_id(),
+    metrics_utils::LogSnoozed(notification_id,
                               /*is_slide_controls=*/true,
                               /*is_popup=*/false);
   }
+
+  // Button handlers of |message_view_| may have closed |this|.
+  if (!weak_this)
+    return;
+
   HideButtons();
 
   // Closing the swipe control is done in these button pressed handlers.

ash/system/message_center/notification_swipe_control_view.h

@@ -6,7 +6,7 @@
 #define ASH_SYSTEM_MESSAGE_CENTER_NOTIFICATION_SWIPE_CONTROL_VIEW_H_
 
 #include "ash/ash_export.h"
-#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/observer_list_types.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ -35,6 +35,9 @@ class ASH_EXPORT NotificationSwipeControlView : public views::View,
 
   explicit NotificationSwipeControlView(
       message_center::MessageView* message_view);
+  NotificationSwipeControlView(const NotificationSwipeControlView&) = delete;
+  NotificationSwipeControlView& operator=(const NotificationSwipeControlView&) =
+      delete;
   ~NotificationSwipeControlView() override;
 
   // views::View
@@ -50,6 +53,11 @@ class ASH_EXPORT NotificationSwipeControlView : public views::View,
   void UpdateCornerRadius(int top_radius, int bottom_radius);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(NotificationSwipeControlViewTest,
+                           DeleteOnSettingsButtonPressed);
+  FRIEND_TEST_ALL_PREFIXES(NotificationSwipeControlViewTest,
+                           DeleteOnSnoozeButtonPressed);
+
   // Change the visibility of the settings button.
   void ShowButtons(ButtonPosition button_position,
                    bool has_settings,
@@ -68,7 +76,7 @@ class ASH_EXPORT NotificationSwipeControlView : public views::View,
   views::ImageButton* settings_button_ = nullptr;
   views::ImageButton* snooze_button_ = nullptr;
 
-  DISALLOW_COPY_AND_ASSIGN(NotificationSwipeControlView);
+  base::WeakPtrFactory<NotificationSwipeControlView> weak_factory_{this};
 };
 
 }  // namespace ash

ash/system/message_center/notification_swipe_control_view_unittest.cc

+// Copyright 2020 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 "ash/system/message_center/notification_swipe_control_view.h"
+
+#include <memory>
+
+#include "base/strings/string16.h"
+#include "base/strings/utf_string_conversions.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/events/base_event_utils.h"
+#include "ui/gfx/geometry/point_f.h"
+#include "ui/gfx/image/image.h"
+#include "ui/message_center/lock_screen/fake_lock_screen_controller.h"
+#include "ui/message_center/message_center.h"
+#include "ui/message_center/public/cpp/notification.h"
+#include "ui/message_center/views/message_view.h"
+#include "ui/message_center/views/notification_control_buttons_view.h"
+#include "url/gurl.h"
+
+namespace {
+
+class MockMessageView : public message_center::MessageView {
+ public:
+  explicit MockMessageView(const message_center::Notification& notification)
+      : message_center::MessageView(notification),
+        buttons_view_(
+            std::make_unique<message_center::NotificationControlButtonsView>(
+                this)) {
+    buttons_view_->ShowSettingsButton(
+        notification.should_show_settings_button());
+    buttons_view_->ShowSnoozeButton(notification.should_show_snooze_button());
+  }
+  ~MockMessageView() override = default;
+
+  message_center::NotificationControlButtonsView* GetControlButtonsView()
+      const override {
+    return buttons_view_.get();
+  }
+
+  MOCK_METHOD(void,
+              OnSettingsButtonPressed,
+              (const ui::Event& event),
+              (override));
+  MOCK_METHOD(void,
+              OnSnoozeButtonPressed,
+              (const ui::Event& event),
+              (override));
+
+ private:
+  std::unique_ptr<message_center::NotificationControlButtonsView> buttons_view_;
+};
+
+}  // namespace
+
+namespace ash {
+
+class NotificationSwipeControlViewTest : public testing::Test {
+ public:
+  NotificationSwipeControlViewTest() = default;
+  ~NotificationSwipeControlViewTest() override = default;
+
+  void SetUp() override {
+    message_center::MessageCenter::Initialize(
+        std::make_unique<message_center::FakeLockScreenController>());
+
+    message_center::RichNotificationData rich_data;
+    rich_data.settings_button_handler =
+        message_center::SettingsButtonHandler::DELEGATE;
+    rich_data.should_show_snooze_button = true;
+    message_center::Notification notification(
+        message_center::NOTIFICATION_TYPE_SIMPLE, "id",
+        base::UTF8ToUTF16("title"), base::UTF8ToUTF16("id"), gfx::Image(),
+        base::string16(), GURL(),
+        message_center::NotifierId(message_center::NotifierType::APPLICATION,
+                                   "notifier_id"),
+        rich_data, nullptr);
+
+    message_view_ = std::make_unique<MockMessageView>(notification);
+  }
+
+  void TearDown() override { message_center::MessageCenter::Shutdown(); }
+
+ protected:
+  MockMessageView* message_view() { return message_view_.get(); }
+
+ private:
+  std::unique_ptr<MockMessageView> message_view_;
+};
+
+TEST_F(NotificationSwipeControlViewTest, DeleteOnSettingsButtonPressed) {
+  auto swipe_control_view =
+      std::make_unique<NotificationSwipeControlView>(message_view());
+
+  EXPECT_CALL(*message_view(), OnSettingsButtonPressed(testing::_))
+      .WillOnce(testing::DoDefault())
+      .WillOnce(
+          testing::InvokeWithoutArgs([&]() { swipe_control_view.reset(); }));
+
+  ui::MouseEvent press(ui::ET_MOUSE_PRESSED, gfx::PointF(), gfx::PointF(),
+                       ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON,
+                       ui::EF_NONE);
+
+  // First click will do nothing, expect that to work.
+  swipe_control_view->ShowButtons(
+      NotificationSwipeControlView::ButtonPosition::LEFT,
+      /*has_settings_button=*/true, /*has_snooze_button=*/true);
+  swipe_control_view->ButtonPressed(swipe_control_view->settings_button_,
+                                    press);
+  EXPECT_TRUE(swipe_control_view);
+
+  // Second click deletes |swipe_control_view| in the handler.
+  swipe_control_view->ShowButtons(
+      NotificationSwipeControlView::ButtonPosition::LEFT,
+      /*has_settings_button=*/true, /*has_snooze_button=*/true);
+  swipe_control_view->ButtonPressed(swipe_control_view->settings_button_,
+                                    press);
+  EXPECT_FALSE(swipe_control_view);
+}
+
+TEST_F(NotificationSwipeControlViewTest, DeleteOnSnoozeButtonPressed) {
+  auto swipe_control_view =
+      std::make_unique<NotificationSwipeControlView>(message_view());
+
+  EXPECT_CALL(*message_view(), OnSnoozeButtonPressed(testing::_))
+      .WillOnce(testing::DoDefault())
+      .WillOnce(
+          testing::InvokeWithoutArgs([&]() { swipe_control_view.reset(); }));
+
+  ui::MouseEvent press(ui::ET_MOUSE_PRESSED, gfx::PointF(), gfx::PointF(),
+                       ui::EventTimeForNow(), ui::EF_LEFT_MOUSE_BUTTON,
+                       ui::EF_NONE);
+
+  // First click will do nothing, expect that to work.
+  swipe_control_view->ShowButtons(
+      NotificationSwipeControlView::ButtonPosition::LEFT,
+      /*has_settings_button=*/true, /*has_snooze_button=*/true);
+  swipe_control_view->ButtonPressed(swipe_control_view->snooze_button_, press);
+  EXPECT_TRUE(swipe_control_view);
+
+  // Second click deletes |swipe_control_view| in the handler.
+  swipe_control_view->ShowButtons(
+      NotificationSwipeControlView::ButtonPosition::LEFT,
+      /*has_settings_button=*/true, /*has_snooze_button=*/true);
+  swipe_control_view->ButtonPressed(swipe_control_view->snooze_button_, press);
+  EXPECT_FALSE(swipe_control_view);
+}
+
+}  // namespace ash

ui/message_center/BUILD.gn

@@ -138,6 +138,8 @@ if (enable_message_center) {
     sources = [
       "fake_message_center.cc",
       "fake_message_center.h",
+      "lock_screen/fake_lock_screen_controller.cc",
+      "lock_screen/fake_lock_screen_controller.h",
     ]
 
     deps = [
@@ -160,8 +162,6 @@ if (enable_message_center) {
     }
 
     sources = [
-      "lock_screen/fake_lock_screen_controller.cc",
-      "lock_screen/fake_lock_screen_controller.h",
       "message_center_impl_unittest.cc",
       "notification_list_unittest.cc",
       "public/cpp/notification_delegate_unittest.cc",