Address a race condition in displaying notifications

content/browser/notifications/platform_notification_context_impl.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 "content/browser/notifications/platform_notification_context_impl.h"
 
 #include "base/bind_helpers.h"
 #include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "base/time/default_clock.h"
 #include "content/browser/notifications/blink_notification_service_impl.h"
 #include "content/browser/notifications/notification_database.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_database_data.h"
 #include "content/public/browser/platform_notification_service.h"
 
 using base::DoNothing;
 
 namespace content {
 namespace {
 
+// Grace period, in seconds, during which we avoid deleting recently shown
+// notifications from the database.
+constexpr double kNotificationDisplayGraceSeconds = 5.0;
+
 // Name of the directory in the user's profile directory where the notification
 // database files should be stored.
 const base::FilePath::CharType kPlatformNotificationsDirectory[] =
     FILE_PATH_LITERAL("Platform Notifications");
 
 }  // namespace
 
 PlatformNotificationContextImpl::PlatformNotificationContextImpl(
     const base::FilePath& path,
     BrowserContext* browser_context,
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context)
     : path_(path),
       browser_context_(browser_context),
       service_worker_context_(service_worker_context),
-      notification_id_generator_(browser_context) {
+      notification_id_generator_(browser_context),
+      clock_(base::MakeUnique<base::DefaultClock>()) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 }
 
 PlatformNotificationContextImpl::~PlatformNotificationContextImpl() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // If the database has been initialized, it must be deleted on the task runner
   // thread as closing it may cause file I/O.
   if (database_) {
     DCHECK(task_runner_);
@@ skipping 213 matching lines @@
 void PlatformNotificationContextImpl::
     DoReadAllNotificationDataForServiceWorkerRegistration(
         const GURL& origin,
         int64_t service_worker_registration_id,
         const ReadAllResultCallback& callback,
         std::unique_ptr<std::set<std::string>> displayed_notifications,
         bool supports_synchronization) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
   DCHECK(displayed_notifications);
 
+  base::Time current_time = clock_->Now();
+
+  // Prune the list of recently shown notifications to those that were displayed
+  // in the past |kNotificationDisplayGraceSeconds| seconds.
+  auto iter = recent_notifications_.begin();
+  while (iter != recent_notifications_.end()) {
+    base::TimeDelta display_time = current_time - iter->second;
+    if (display_time.InSecondsF() > kNotificationDisplayGraceSeconds)
+      iter = recent_notifications_.erase(iter);
+    else
+      iter++;
+  }
+
   std::vector<NotificationDatabaseData> notification_datas;
 
   NotificationDatabase::Status status =
       database_->ReadAllNotificationDataForServiceWorkerRegistration(
           origin, service_worker_registration_id, &notification_datas);
 
   UMA_HISTOGRAM_ENUMERATION("Notifications.Database.ReadForServiceWorkerResult",
                             status, NotificationDatabase::STATUS_COUNT);
 
   std::vector<std::string> obsolete_notifications;
 
   if (status == NotificationDatabase::STATUS_OK) {
     if (supports_synchronization) {
       for (auto it = notification_datas.begin();
            it != notification_datas.end();) {
         // The database is only used for persistent notifications.
         DCHECK(NotificationIdGenerator::IsPersistentNotification(
             it->notification_id));
-        if (displayed_notifications->count(it->notification_id)) {
+
+        const bool notification_exists =
+            displayed_notifications->count(it->notification_id) ||
+            recent_notifications_.count(it->notification_id);
+
+        if (notification_exists) {
           ++it;
         } else {
           obsolete_notifications.push_back(it->notification_id);
           it = notification_datas.erase(it);
         }
       }
     }
 
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
@@ skipping 64 matching lines @@
           origin, database_data.notification_data.tag,
           database_->GetNextPersistentNotificationId());
 
   NotificationDatabase::Status status =
       database_->WriteNotificationData(origin, write_database_data);
 
   UMA_HISTOGRAM_ENUMERATION("Notifications.Database.WriteResult", status,
                             NotificationDatabase::STATUS_COUNT);
 
   if (status == NotificationDatabase::STATUS_OK) {
+    recent_notifications_[write_database_data.notification_id] = clock_->Now();
+
     BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                             base::Bind(callback, true /* success */,
                                        write_database_data.notification_id));
 
     return;
   }
 
   // Blow away the database if writing data failed due to corruption.
   if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED)
     DestroyDatabase();
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(callback, false /* success */, "" /* notification_id */));
 }
 
 void PlatformNotificationContextImpl::DeleteNotificationData(

[awdf, 2017/05/11 10:38:53]

Just to check my understanding - this DeleteNotificationData method should get
called for every notification, right?

     const std::string& notification_id,
     const GURL& origin,
     const DeleteResultCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   LazyInitialize(
       base::Bind(&PlatformNotificationContextImpl::DoDeleteNotificationData,
                  this, notification_id, origin, callback),
       base::Bind(callback, false /* success */));
 }
 
 void PlatformNotificationContextImpl::DoDeleteNotificationData(
     const std::string& notification_id,
     const GURL& origin,
     const DeleteResultCallback& callback) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   NotificationDatabase::Status status =
       database_->DeleteNotificationData(notification_id, origin);
 
   UMA_HISTOGRAM_ENUMERATION("Notifications.Database.DeleteResult", status,
                             NotificationDatabase::STATUS_COUNT);
 
+  recent_notifications_.erase(notification_id);
+
   bool success = status == NotificationDatabase::STATUS_OK;
 
   // Blow away the database if deleting data failed due to corruption. Following
   // the contract of the delete methods, consider this to be a success as the
   // caller's goal has been achieved: the data is gone.
   if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED) {
     DestroyDatabase();
     success = true;
   }
 
@@ skipping 140 matching lines @@
 
   return path_.Append(kPlatformNotificationsDirectory);
 }
 
 void PlatformNotificationContextImpl::SetTaskRunnerForTesting(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
   task_runner_ = task_runner;
 }
 
 }  // namespace content