Address a race condition in displaying notifications

content/browser/notifications/platform_notification_context_unittest.cc

Index: content/browser/notifications/platform_notification_context_unittest.cc
diff --git a/content/browser/notifications/platform_notification_context_unittest.cc b/content/browser/notifications/platform_notification_context_unittest.cc
index 945fd16a4f7328d45a99f251fd107899dc7b1665..42f6bef89870226c5a6baf3a7d3b57eaad2222ff 100644
--- a/content/browser/notifications/platform_notification_context_unittest.cc
+++ b/content/browser/notifications/platform_notification_context_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/notifications/platform_notification_context_impl.h"
 #include "content/browser/service_worker/embedded_worker_test_helper.h"
@@ -85,15 +86,29 @@ class PlatformNotificationContextTest : public ::testing::Test {
 
   // Callback to provide when reading multiple notifications from the database.
   // Will store the success value in the class member, and write the read
-  // notification datas to |store_notification_datas|.
+  // notification datas to |*out_notification_datas|.
   void DidReadAllNotificationDatas(
-      std::vector<NotificationDatabaseData>* store_notification_datas,
+      std::vector<NotificationDatabaseData>* out_notification_datas,
       bool success,
       const std::vector<NotificationDatabaseData>& notification_datas) {
-    DCHECK(store_notification_datas);
+    DCHECK(out_notification_datas);
 
     success_ = success;
-    *store_notification_datas = notification_datas;
+    *out_notification_datas = notification_datas;
+  }
+
+  // Callback to provide when calling GetDisplayedNotifications on the
+  // PlatformNotificationService.
+  void DidGetDisplayedNotifications(
+      std::unique_ptr<std::set<std::string>>* out_notifications,
+      bool* out_supports_synchronization,
+      std::unique_ptr<std::set<std::string>> notifications,
+      bool supports_synchronization) {
+    DCHECK(out_notifications);
+    DCHECK(out_supports_synchronization);
+
+    *out_notifications = std::move(notifications);
+    *out_supports_synchronization = supports_synchronization;
   }
 
  protected:
@@ -109,6 +124,15 @@ class PlatformNotificationContextTest : public ::testing::Test {
     return context;
   }
 
+  // Creates and returns a fake clock for the |context| that can be used to
+  // advance the time. Used by tests that care about the deletion grace period.
+  base::SimpleTestClock* CreateFakeClock(
+      PlatformNotificationContextImpl* context) {
+    base::SimpleTestClock* clock = new base::SimpleTestClock();
+    context->clock_ = base::WrapUnique(clock);
+    return clock;
+  }
+
   // Overrides the task runner in |context| with the current message loop
   // proxy, to reduce the number of threads involved in the tests.
   void OverrideTaskRunnerForTesting(PlatformNotificationContextImpl* context) {
@@ -507,6 +531,8 @@ TEST_F(PlatformNotificationContextTest, SynchronizeNotifications) {
   scoped_refptr<PlatformNotificationContextImpl> context =
       CreatePlatformNotificationContext();
 
+  base::SimpleTestClock* clock = CreateFakeClock(context.get());
+
   GURL origin("https://example.com");
   NotificationDatabaseData notification_database_data;
   notification_database_data.service_worker_registration_id =
@@ -523,6 +549,11 @@ TEST_F(PlatformNotificationContextTest, SynchronizeNotifications) {
   ASSERT_TRUE(success());
   EXPECT_FALSE(notification_id().empty());
 
+  // Forward the time by a minute to ensure that we're past the grace period for
+  // newly shown notifications, making sure that the list of displayed
+  // notifications retrieved from the PlatformNotificationService will be used.
+  clock->Advance(base::TimeDelta::FromMinutes(1));
+
   PlatformNotificationService* service =
       notification_browser_client.GetPlatformNotificationService();
 
@@ -565,4 +596,83 @@ TEST_F(PlatformNotificationContextTest, SynchronizeNotifications) {
   EXPECT_FALSE(success());
 }
 
+TEST_F(PlatformNotificationContextTest, SynchronizeNotificationsDisplayRace) {
+  // This test verifies that a possible race condition where a developer shows
+  // a notification, and then gets all shown notifications before waiting for
+  // acknowledgement (which, in itself, is too optimistic), is made less severe
+  // by remembering the most recently shown notification for a brief period of
+  // time.
+
+  NotificationBrowserClient notification_browser_client;
+  SetBrowserClientForTesting(&notification_browser_client);
+
+  scoped_refptr<PlatformNotificationContextImpl> context =
+      CreatePlatformNotificationContext();
+
+  base::SimpleTestClock* clock = CreateFakeClock(context.get());
+
+  GURL origin("https://example.com");
+  NotificationDatabaseData notification_database_data;
+  notification_database_data.service_worker_registration_id =
+      kFakeServiceWorkerRegistrationId;
+  PlatformNotificationData notification_data;
+  content::NotificationResources notification_resources;
+
+  context->WriteNotificationData(
+      origin, notification_database_data,
+      base::Bind(&PlatformNotificationContextTest::DidWriteNotificationData,
+                 base::Unretained(this)));
+
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(success());
+  EXPECT_FALSE(notification_id().empty());
+
+  // The |notification_data| deliberately has not been dispatched to the
+  // PlatformNotificationService yet. This will cause the mock to return that
+  // no notifications have been displayed, simulating the race condition.
+
+  std::unique_ptr<std::set<std::string>> notifications;
+  bool supports_synchronization = false;
+
+  PlatformNotificationService* service =
+      notification_browser_client.GetPlatformNotificationService();
+  service->GetDisplayedNotifications(
+      browser_context(),
+      base::Bind(&PlatformNotificationContextTest::DidGetDisplayedNotifications,
+                 base::Unretained(this), &notifications,
+                 &supports_synchronization));
+
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(supports_synchronization);
+  ASSERT_EQ(notifications->size(), 0u);
+
+  // Reading all notifications should still return the one notification that was
+  // displayed, since it was the most recent one.
+
+  std::vector<NotificationDatabaseData> notification_database_datas;
+  context->ReadAllNotificationDataForServiceWorkerRegistration(
+      origin, kFakeServiceWorkerRegistrationId,
+      base::Bind(&PlatformNotificationContextTest::DidReadAllNotificationDatas,
+                 base::Unretained(this), &notification_database_datas));
+
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_TRUE(success());
+  ASSERT_EQ(1u, notification_database_datas.size());
+
+  // Now forward the time by a minute, which means that the grace period should
+  // have exceeded. Getting the notifications again should ignore it now.
+  clock->Advance(base::TimeDelta::FromMinutes(1));
+
+  context->ReadAllNotificationDataForServiceWorkerRegistration(
+      origin, kFakeServiceWorkerRegistrationId,
+      base::Bind(&PlatformNotificationContextTest::DidReadAllNotificationDatas,
+                 base::Unretained(this), &notification_database_datas));
+
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_TRUE(success());
+  ASSERT_EQ(0u, notification_database_datas.size());
+}
+
 }  // namespace content