Fix the crash bug of close button for the resolution change notification.

Close() is invoked in RemoveNotification() before removing the
notification (because the NotificationDelegate is owned by its notification).
Invoking AcceptResolutionChange() directly here causes the removing
notification twice, which will cause a crash.

BUG=271784
R=oshima@chromium.org
TEST=covered by the new test case

Review URL: https://chromiumcodereview.appspot.com/22960004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@217338 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 46 insertions, 8 deletions

diff --git a/ash/display/resolution_notification_controller.cc b/ash/display/resolution_notification_controller.cc
index a765d61..e2755a0 100644
--- a/ash/display/resolution_notification_controller.cc
+++ b/ash/display/resolution_notification_controller.cc
@@ -71,11 +71,11 @@ void ResolutionChangeNotificationDelegate::Error() {
 
 void ResolutionChangeNotificationDelegate::Close(bool by_user) {
   if (by_user)
-    controller_->AcceptResolutionChange();
+    controller_->AcceptResolutionChange(false);
 }
 
 void ResolutionChangeNotificationDelegate::Click() {
-  controller_->AcceptResolutionChange();
+  controller_->AcceptResolutionChange(true);
 }
 
 bool ResolutionChangeNotificationDelegate::HasClickedListener() {
@@ -86,7 +86,7 @@ void ResolutionChangeNotificationDelegate::ButtonClick(int button_index) {
   // If there's the timeout, the first button is "Accept". Otherwise the
   // button click should be "Revert".
   if (has_timeout_ && button_index == 0)
-    controller_->AcceptResolutionChange();
+    controller_->AcceptResolutionChange(true);
   else
     controller_->RevertResolutionChange();
 }
@@ -243,9 +243,12 @@ void ResolutionNotificationController::OnTimerTick() {
     CreateOrUpdateNotification();
 }
 
-void ResolutionNotificationController::AcceptResolutionChange() {
-  message_center::MessageCenter::Get()->RemoveNotification(
-      kNotificationId, false /* by_user */);
+void ResolutionNotificationController::AcceptResolutionChange(
+    bool close_notification) {
+  if (close_notification) {
+    message_center::MessageCenter::Get()->RemoveNotification(
+        kNotificationId, false /* by_user */);
+  }
   base::Closure callback = change_info_->accept_callback;
   change_info_.reset();
   callback.Run();
diff --git a/ash/display/resolution_notification_controller.h b/ash/display/resolution_notification_controller.h
index 4de39c2..14e6b59 100644
--- a/ash/display/resolution_notification_controller.h
+++ b/ash/display/resolution_notification_controller.h
@@ -50,8 +50,9 @@ class ASH_EXPORT ResolutionNotificationController
   bool DoesNotificationTimeout();
 
   // Called by the notification delegate when the user accepts the display
-  // resolution change.
-  void AcceptResolutionChange();
+  // resolution change. Set |close_notification| to true when the notification
+  // should be removed.
+  void AcceptResolutionChange(bool close_notification);
 
   // Called by the notification delegate when the user wants to revert the
   // display resolution change.
diff --git a/ash/display/resolution_notification_controller_unittest.cc b/ash/display/resolution_notification_controller_unittest.cc
index 1ae1750..036cce0 100644
--- a/ash/display/resolution_notification_controller_unittest.cc
+++ b/ash/display/resolution_notification_controller_unittest.cc
@@ -68,6 +68,11 @@ class ResolutionNotificationControllerTest : public ash::test::AshTestBase {
         ResolutionNotificationController::kNotificationId, index);
   }
 
+  void CloseNotification() {
+    message_center::MessageCenter::Get()->RemoveNotification(
+        ResolutionNotificationController::kNotificationId, true /* by_user */);
+  }
+
   bool IsNotificationVisible() {
     return message_center::MessageCenter::Get()->HasNotification(
         ResolutionNotificationController::kNotificationId);
@@ -194,6 +199,35 @@ TEST_F(ResolutionNotificationControllerTest, AcceptButton) {
       display.id(), &resolution));
 }
 
+TEST_F(ResolutionNotificationControllerTest, Close) {
+  if (!SupportsMultipleDisplays())
+    return;
+
+  UpdateDisplay("100x100,150x150#150x150|200x200");
+  int64 id2 = ash::ScreenAsh::GetSecondaryDisplay().id();
+  ash::internal::DisplayManager* display_manager =
+      ash::Shell::GetInstance()->display_manager();
+  ASSERT_EQ(0, accept_count());
+  EXPECT_FALSE(IsNotificationVisible());
+
+  // Changes the resolution and apply the result.
+  SetDisplayResolutionAndNotify(
+      ScreenAsh::GetSecondaryDisplay(), gfx::Size(200, 200));
+  EXPECT_TRUE(IsNotificationVisible());
+  EXPECT_FALSE(controller()->DoesNotificationTimeout());
+  gfx::Size resolution;
+  EXPECT_TRUE(
+      display_manager->GetSelectedResolutionForDisplayId(id2, &resolution));
+  EXPECT_EQ("200x200", resolution.ToString());
+
+  // Close the notification (imitates clicking [x] button). Also verifies if
+  // this does not cause a crash.  See crbug.com/271784
+  CloseNotification();
+  RunAllPendingInMessageLoop();
+  EXPECT_FALSE(IsNotificationVisible());
+  EXPECT_EQ(1, accept_count());
+}
+
 TEST_F(ResolutionNotificationControllerTest, Timeout) {
   if (!SupportsMultipleDisplays())
     return;