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();
@@ skipping 437 matching lines @@
   // Delete the notification from the display service without removing it from
   // the database. It should automatically synchronize on the next read.
   service->ClosePersistentNotification(browser_context(), notification_id());
   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());

[awdf, 2017/05/10 17:01:57]

This line is now failing - see
https://00e9e64bacafae9f30f2cf269632054d540ffb500e728c90aa-apidata.googleuser...


Guess you'll need a test clock in this test too to fake time fast-forwarding.

[Peter Beverloo, 2017/05/10 17:09:22]

Done.

 
   context->ReadNotificationData(
       notification_id(), origin,
       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 = new base::SimpleTestClock();
+  context->clock_ = base::WrapUnique(clock);
+
+  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