Merge 217338 "Fix the crash bug of close button for the resoluti..."

> 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

TBR=mukai@chromium.org

Review URL: https://codereview.chromium.org/23049003

git-svn-id: svn://svn.chromium.org/chrome/branches/1599/src@217585 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 bdb6f12..3a479f9 100644
--- a/ash/display/resolution_notification_controller_unittest.cc
+++ b/ash/display/resolution_notification_controller_unittest.cc
@@ -66,6 +66,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) {
   EXPECT_EQ("100x100", resolution.ToString());
 }
 
+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;