Fix the FullRestoreServiceTest unit tests.

The parameter id of the function HandleRestoreNotificationClicked is
user-after-free on builder "Linux ChromiumOS MSan Tests", because the
notification is closed at the beginning of
HandleRestoreNotificationClicked, which may cause `id` becomes
uninitialized.

Move notification_ as the class variable to hold the notitification id
in the function HandleRestoreNotificationClicked to resolve the unit
tests issue. And enable the disabled unit tests.

BUG=1146900
BUG=1164559

Change-Id: Iabf8ad0233944cce071b3c2be1a10ca746ed2a6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2619409
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842107}

chrome/browser/chromeos/full_restore/full_restore_service.cc

@@ -113,38 +113,37 @@ void FullRestoreService::ShowRestoreNotification(const std::string& id) {
   else
     message_id = IDS_SET_RESTORE_NOTIFICATION_MESSAGE;
 
-  std::unique_ptr<message_center::Notification> notification =
-      ash::CreateSystemNotification(
-          message_center::NOTIFICATION_TYPE_SIMPLE, id,
-          l10n_util::GetStringUTF16(title_id),
-          l10n_util::GetStringUTF16(message_id),
-          l10n_util::GetStringUTF16(IDS_RESTORE_NOTIFICATION_DISPLAY_SOURCE),
-          GURL(),
-          message_center::NotifierId(
-              message_center::NotifierType::SYSTEM_COMPONENT, id),
-          notification_data,
-          base::MakeRefCounted<message_center::HandleNotificationClickDelegate>(
-              base::BindRepeating(
-                  &FullRestoreService::HandleRestoreNotificationClicked,
-                  weak_ptr_factory_.GetWeakPtr(), id)),
-          kFullRestoreNotificationIcon,
-          message_center::SystemNotificationWarningLevel::NORMAL);
-  notification->set_priority(message_center::SYSTEM_PRIORITY);
+  notification_ = ash::CreateSystemNotification(
+      message_center::NOTIFICATION_TYPE_SIMPLE, id,
+      l10n_util::GetStringUTF16(title_id),
+      l10n_util::GetStringUTF16(message_id),
+      l10n_util::GetStringUTF16(IDS_RESTORE_NOTIFICATION_DISPLAY_SOURCE),
+      GURL(),
+      message_center::NotifierId(message_center::NotifierType::SYSTEM_COMPONENT,
+                                 id),
+      notification_data,
+      base::MakeRefCounted<message_center::HandleNotificationClickDelegate>(
+          base::BindRepeating(
+              &FullRestoreService::HandleRestoreNotificationClicked,
+              weak_ptr_factory_.GetWeakPtr())),
+      kFullRestoreNotificationIcon,
+      message_center::SystemNotificationWarningLevel::NORMAL);
+  notification_->set_priority(message_center::SYSTEM_PRIORITY);
 
   auto* notification_display_service =
       NotificationDisplayService::GetForProfile(profile_);
   DCHECK(notification_display_service);
   notification_display_service->Display(NotificationHandler::Type::TRANSIENT,
-                                        *notification,
+                                        *notification_,
                                         /*metadata=*/nullptr);
 }
 
 void FullRestoreService::HandleRestoreNotificationClicked(
-    const std::string& id,
     base::Optional<int> button_index) {
+  DCHECK(notification_);
   if (!is_shut_down_) {
     NotificationDisplayService::GetForProfile(profile_)->Close(
-        NotificationHandler::Type::TRANSIENT, id);
+        NotificationHandler::Type::TRANSIENT, notification_->id());
   }
 
   if (!button_index.has_value() ||
@@ -153,7 +152,7 @@ void FullRestoreService::HandleRestoreNotificationClicked(
     return;
   }
 
-  if (id == kSetRestorePrefNotificationId) {
+  if (notification_->id() == kSetRestorePrefNotificationId) {
     // Show the 'On Startup' OS setting page.
     chrome::SettingsWindowManager::GetInstance()->ShowOSSettings(
         profile_, chromeos::settings::mojom::kOnStartupSectionPath);

chrome/browser/chromeos/full_restore/full_restore_service.h

@@ -14,6 +14,10 @@
 
 class Profile;
 
+namespace message_center {
+class Notification;
+}
+
 namespace chromeos {
 namespace full_restore {
 
@@ -56,8 +60,7 @@ class FullRestoreService : public KeyedService {
   // Show the restore notification on startup.
   void ShowRestoreNotification(const std::string& id);
 
-  void HandleRestoreNotificationClicked(const std::string& id,
-                                        base::Optional<int> button_index);
+  void HandleRestoreNotificationClicked(base::Optional<int> button_index);
 
   // Implement the restoration.
   void Restore();
@@ -74,6 +77,8 @@ class FullRestoreService : public KeyedService {
 
   std::unique_ptr<FullRestoreDataHandler> restore_data_handler_;
 
+  std::unique_ptr<message_center::Notification> notification_;
+
   base::WeakPtrFactory<FullRestoreService> weak_ptr_factory_{this};
 };
 

chrome/browser/chromeos/full_restore/full_restore_service_unittest.cc

@@ -189,8 +189,7 @@ class FullRestoreServiceTest : public testing::Test {
 
 // If the system is crash, show the crash notification, and verify the restore
 // flag when click the restore button.
-// TODO(crbug.com/1046900): Fix this unit test.
-TEST_F(FullRestoreServiceTest, DISABLED_CrashAndRestore) {
+TEST_F(FullRestoreServiceTest, CrashAndRestore) {
   profile()->set_last_session_exited_cleanly(false);
   CreateFullRestoreServiceForTesting();
 
@@ -400,8 +399,7 @@ TEST_F(FullRestoreServiceTest, Upgrading) {
 
 // If the OS restore setting is 'Ask every time', after reboot, show the restore
 // notification, and verify the restore flag when click the restore button.
-// TODO(crbug.com/1046900): Fix this unit test.
-TEST_F(FullRestoreServiceTest, DISABLED_AskEveryTimeAndRestore) {
+TEST_F(FullRestoreServiceTest, AskEveryTimeAndRestore) {
   profile()->GetPrefs()->SetInteger(
       kRestoreAppsAndPagesPrefName,
       static_cast<int>(RestoreOption::kAskEveryTime));
@@ -476,8 +474,7 @@ TEST_F(FullRestoreServiceTest, NotRestore) {
 
 // If the restore option has been selected 3 times, show the set restore
 // notification.
-// TODO(crbug.com/1046900): Fix this unit test.
-TEST_F(FullRestoreServiceTest, DISABLED_SetRestorePrefNotification) {
+TEST_F(FullRestoreServiceTest, SetRestorePrefNotification) {
   profile()->GetPrefs()->SetInteger(
       kRestoreAppsAndPagesPrefName,
       static_cast<int>(RestoreOption::kAskEveryTime));
@@ -511,8 +508,7 @@ TEST_F(FullRestoreServiceTest, DISABLED_SetRestorePrefNotification) {
 
 // When |kRestoreSelectedCountPrefName| = 3, if the restore option is selected
 // again, |kRestoreSelectedCountPrefName| should not change.
-// TODO(crbug.com/1046900): Fix this unit test.
-TEST_F(FullRestoreServiceTest, DISABLED_RestoreSelectedCount) {
+TEST_F(FullRestoreServiceTest, RestoreSelectedCount) {
   profile()->GetPrefs()->SetInteger(
       kRestoreAppsAndPagesPrefName,
       static_cast<int>(RestoreOption::kAskEveryTime));