Address a race condition in displaying notifications

content/browser/notifications/platform_notification_context_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stdint.h>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #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"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/common/service_worker/service_worker_types.h"
 #include "content/public/browser/notification_database_data.h"
 #include "content/public/common/notification_resources.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/test/mock_platform_notification_service.h"
@@ skipping 56 matching lines @@
   // Callback to provide when unregistering a Service Worker. Will write the
   // resulting status code to |store_status|.
   void DidUnregisterServiceWorker(ServiceWorkerStatusCode* store_status,
                                   ServiceWorkerStatusCode status) {
     DCHECK(store_status);
     *store_status = status;
   }
 
   // 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:
   // Creates a new PlatformNotificationContextImpl instance. When using this
   // method, the underlying database will always be created in memory.
   PlatformNotificationContextImpl* CreatePlatformNotificationContext() {
     PlatformNotificationContextImpl* context =
         new PlatformNotificationContextImpl(base::FilePath(), &browser_context_,
                                             nullptr);
     context->Initialize();
 
     OverrideTaskRunnerForTesting(context);
     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) {
     context->SetTaskRunnerForTesting(base::ThreadTaskRunnerHandle::Get());
   }
 
   // Returns the testing browsing context that can be used for this test.
   BrowserContext* browser_context() { return &browser_context_; }
 
   // Returns whether the last invoked callback finished successfully.
@@ skipping 378 matching lines @@
   }
 }
 
 TEST_F(PlatformNotificationContextTest, SynchronizeNotifications) {
   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());
 
+  // 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();
 
   service->DisplayPersistentNotification(browser_context(), notification_id(),
                                          origin, origin, notification_data,
                                          notification_resources);
 
   std::vector<NotificationDatabaseData> notification_database_datas;
   context->ReadAllNotificationDataForServiceWorkerRegistration(
       origin, kFakeServiceWorkerRegistrationId,
@@ skipping 22 matching lines @@
       base::Bind(&PlatformNotificationContextTest::DidReadNotificationData,
                  base::Unretained(this)));
 
   base::RunLoop().RunUntilIdle();
 
   // The notification was removed, so we shouldn't be able to read it from
   // the database anymore.
   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