Delay deleting profile notification

BUG=703113
TEST=create html notification, exit browser, wait until notification closes and/or close notification.

Review-Url: https://codereview.chromium.org/2803593003
Cr-Commit-Position: refs/heads/master@{#463377}

chrome/browser/chromeos/hats/hats_notification_controller_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/hats/hats_notification_controller.h"
 
+#include "base/run_loop.h"
 #include "base/strings/string_split.h"
 #include "chrome/browser/notifications/message_center_notification_manager.h"
 #include "chrome/browser/notifications/notification.h"
@@ -113,6 +114,9 @@ class HatsNotificationControllerTest : public BrowserWithTestWindowTest {
 
   void TearDown() override {
     g_browser_process->notification_ui_manager()->StartShutdown();
+    // The notifications may be deleted async.
+    base::RunLoop loop;
+    loop.RunUntilIdle();
     profile_manager_.reset();
     network_portal_detector::InitializeForTesting(nullptr);
     BrowserWithTestWindowTest::TearDown();

chrome/browser/notifications/message_center_notification_manager.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/notifications/profile_notification.h"
 #include "chrome/browser/notifications/screen_lock_notification_blocker.h"
 #include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/browser/extension_registry.h"
@@ -329,18 +330,19 @@ void MessageCenterNotificationManager::RemoveProfileNotification(
   if (it == profile_notifications_.end())
     return;
 
-  // Delay destruction of the ProfileNotification until after all the work
-  // removing it from |profile_notifications_| is complete. This must be done
-  // because this ProfileNotification might have the one ScopedKeepAlive object
-  // that was keeping the browser alive, and destroying it would result in a re-
-  // entrant call to this class. Because every method in this class touches
-  // |profile_notifications_|, |profile_notifications_| must always be in a
-  // self-consistent state in moments where re-entrance might happen.
-  // https://crbug.com/649971
-  std::unique_ptr<ProfileNotification> notification = std::move(it->second);
+  // Delay destruction of the ProfileNotification until current task is
+  // completed. This must be done because this ProfileNotification might have
+  // the one ScopedKeepAlive object that was keeping the browser alive, and
+  // destroying it would result in:
+  // a) A reentrant call to this class. Because every method in this class
+  //   touches |profile_notifications_|, |profile_notifications_| must always
+  //   be in a self-consistent state in moments where re-entrance might happen.
+  // b) A crash like https://crbug.com/649971 because it can trigger
+  //    shutdown process while we're still inside the call stack from UI
+  //    framework.
+  content::BrowserThread::DeleteSoon(content::BrowserThread::UI, FROM_HERE,
+                                     it->second.release());
   profile_notifications_.erase(it);
-  // Now that the map modifications are complete, going out of scope will
-  // destroy the notification.
 }
 
 ProfileNotification* MessageCenterNotificationManager::FindProfileNotification(

chrome/browser/notifications/message_center_notifications_browsertest.cc

@@ -8,6 +8,7 @@
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -139,6 +140,11 @@ class MessageCenterNotificationsTest : public InProcessBrowserTest {
         base::UTF8ToUTF16("chrome-test://testing/"),
         GURL("chrome-test://testing/"), "REPLACE-ME", data, new_delegate);
   }
+
+  void RunLoopUntilIdle() {
+    base::RunLoop loop;
+    loop.RunUntilIdle();
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest, RetrieveBaseParts) {
@@ -201,19 +207,23 @@ IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest, VerifyKeepAlives) {
 
   TestDelegate* delegate;
   manager()->Add(CreateTestNotification("a", &delegate), profile());
+  RunLoopUntilIdle();
   EXPECT_TRUE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
       KeepAliveOrigin::NOTIFICATION));
 
   TestDelegate* delegate2;
   manager()->Add(CreateRichTestNotification("b", &delegate2), profile());
+  RunLoopUntilIdle();
   EXPECT_TRUE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
       KeepAliveOrigin::NOTIFICATION));
 
   manager()->CancelById("a", NotificationUIManager::GetProfileID(profile()));
+  RunLoopUntilIdle();
   EXPECT_TRUE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
       KeepAliveOrigin::NOTIFICATION));
 
   manager()->CancelById("b", NotificationUIManager::GetProfileID(profile()));
+  RunLoopUntilIdle();
   EXPECT_FALSE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
       KeepAliveOrigin::NOTIFICATION));