Postpone listener replacement after non-persistent notification closed

When calling |RegisterNonPersistentNotificationListener| with an existed
notification id, it will first dispatch a close event to old notification
and set new listener into listener map.

Currently the new listener is replaced immediately after calling
|DispatchNonPersistentCloseEvent|, since it's an asynchronize call,
it will remove the replaced listener unexpectedly, instead of removing
the original listener.

To fix this, we can postpone listener replacement after the close event
finish it's cleaning job. For new created notification, the listener can
be set immediately due to there is no close event need to be dispatched.

Bug: 880266, 898486
Change-Id: I370d77386cf1a6636f368f1785d92d362a3b4263
Reviewed-on: https://chromium-review.googlesource.com/c/1303898Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604203}

content/browser/notifications/notification_event_dispatcher_impl.cc

@@ -426,6 +426,14 @@ void NotificationEventDispatcherImpl::DispatchNotificationCloseEvent(
 void NotificationEventDispatcherImpl::RegisterNonPersistentNotificationListener(
     const std::string& notification_id,
     blink::mojom::NonPersistentNotificationListenerPtr event_listener_ptr) {
+  // Observe connection errors, which occur when the JavaScript object or the
+  // renderer hosting them goes away. (For example through navigation.) The
+  // listener gets freed together with |this|, thus the Unretained is safe.
+  event_listener_ptr.set_connection_error_handler(base::BindOnce(
+      &NotificationEventDispatcherImpl::
+          HandleConnectionErrorForNonPersistentNotificationListener,
+      base::Unretained(this), notification_id));
+
   if (non_persistent_notification_listeners_.count(notification_id)) {
     // Dispatch the close event for any previously displayed notification with
     // the same notification id. This happens whenever a non-persistent
@@ -433,17 +441,15 @@ void NotificationEventDispatcherImpl::RegisterNonPersistentNotificationListener(
     // from the JavaScript point of view there will be two notification objects,
     // and the old one needs to receive a close event before the new one
     // receives a show event.
-    DispatchNonPersistentCloseEvent(notification_id, base::DoNothing());
+    DispatchNonPersistentCloseEvent(
+        notification_id,
+        base::BindOnce(&NotificationEventDispatcherImpl::
+                           ReplaceNonPersistentNotificationListener,
+                       base::Unretained(this), notification_id,
+                       std::move(event_listener_ptr)));
+    return;
   }
 
-  // Observe connection errors, which occur when the JavaScript object or the
-  // renderer hosting them goes away. (For example through navigation.) The
-  // listener gets freed together with |this|, thus the Unretained is safe.
-  event_listener_ptr.set_connection_error_handler(base::BindOnce(
-      &NotificationEventDispatcherImpl::
-          HandleConnectionErrorForNonPersistentNotificationListener,
-      base::Unretained(this), notification_id));
-
   non_persistent_notification_listeners_.emplace(notification_id,
                                                  std::move(event_listener_ptr));
 }
@@ -489,6 +495,13 @@ void NotificationEventDispatcherImpl::OnNonPersistentCloseComplete(
   std::move(completed_closure).Run();
 }
 
+void NotificationEventDispatcherImpl::ReplaceNonPersistentNotificationListener(
+    const std::string& notification_id,
+    blink::mojom::NonPersistentNotificationListenerPtr event_listener_ptr) {
+  non_persistent_notification_listeners_.emplace(notification_id,
+                                                 std::move(event_listener_ptr));
+}
+
 void NotificationEventDispatcherImpl::
     HandleConnectionErrorForNonPersistentNotificationListener(
         const std::string& notification_id) {

content/browser/notifications/notification_event_dispatcher_impl.h

@@ -67,6 +67,13 @@ class CONTENT_EXPORT NotificationEventDispatcherImpl
   void OnNonPersistentCloseComplete(const std::string& notification_id,
                                     base::OnceClosure completed_closure);
 
+  // Replace listener for existing non-persistent notification.
+  // This method is called after a non-persistent notification has
+  // been replaced and |OnNonPersistentCloseComplete| is executed.
+  void ReplaceNonPersistentNotificationListener(
+      const std::string& notification_id,
+      blink::mojom::NonPersistentNotificationListenerPtr event_listener_ptr);
+
   // Removes all references to the listener registered to receive events
   // from the non-persistent notification identified by |notification_id|.
   // Should be called when the connection to this listener goes away.

content/browser/notifications/notification_event_dispatcher_impl_unittest.cc

@@ -119,7 +119,7 @@ TEST_F(NotificationEventDispatcherImplTest,
 }
 
 TEST_F(NotificationEventDispatcherImplTest,
-       RegisterReplacementNonPersistentListener_FirstListenerGetsOnClose) {
+       RegisterNonPersistentListener_FirstListenerGetsOnClose) {
   auto original_listener = std::make_unique<TestNotificationListener>();
   dispatcher_->RegisterNonPersistentNotificationListener(
       kPrimaryUniqueId, original_listener->GetPtr());
@@ -138,6 +138,31 @@ TEST_F(NotificationEventDispatcherImplTest,
   EXPECT_EQ(replacement_listener->on_close_count(), 0);
 }
 
+TEST_F(NotificationEventDispatcherImplTest,
+       RegisterNonPersistentListener_ReplacedListenerGetsOnClick) {
+  auto original_listener = std::make_unique<TestNotificationListener>();
+  dispatcher_->RegisterNonPersistentNotificationListener(
+      kPrimaryUniqueId, original_listener->GetPtr());
+
+  dispatcher_->DispatchNonPersistentShowEvent(kPrimaryUniqueId);
+
+  auto replacement_listener = std::make_unique<TestNotificationListener>();
+  dispatcher_->RegisterNonPersistentNotificationListener(
+      kPrimaryUniqueId, replacement_listener->GetPtr());
+
+  WaitForMojoTasksToComplete();
+
+  dispatcher_->DispatchNonPersistentClickEvent(kPrimaryUniqueId,
+                                               base::DoNothing());
+
+  WaitForMojoTasksToComplete();
+
+  EXPECT_EQ(original_listener->on_click_count(), 0);
+  EXPECT_EQ(original_listener->on_close_count(), 1);
+  EXPECT_EQ(replacement_listener->on_click_count(), 1);
+  EXPECT_EQ(replacement_listener->on_close_count(), 0);
+}
+
 TEST_F(NotificationEventDispatcherImplTest,
        DispatchNonPersistentClickEvent_NotifiesCorrectRegisteredListener) {
   auto listener = std::make_unique<TestNotificationListener>();