The platform notification context should observe the Service Worker Context.

content/browser/notifications/platform_notification_context_impl.cc

Index: content/browser/notifications/platform_notification_context_impl.cc
diff --git a/content/browser/notifications/platform_notification_context_impl.cc b/content/browser/notifications/platform_notification_context_impl.cc
index 22784de527e7c9c151f83e10fee6a92d67eee285..df744ea6b839c8d6d72cdb955dc6b9430df25aec 100644
--- a/content/browser/notifications/platform_notification_context_impl.cc
+++ b/content/browser/notifications/platform_notification_context_impl.cc
@@ -6,10 +6,17 @@
 
 #include "base/threading/sequenced_worker_pool.h"
 #include "content/browser/notifications/notification_database.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_database_data.h"
 
 namespace content {
+namespace {
+
+// Used as a failure callback there is no further action to be made.
+void EmptyFailureCallback() {}

[johnme, 2015/03/20 14:56:35]

Use DoNothing from base/bind_helpers.h instead.

[Peter Beverloo, 2015/03/20 18:39:06]

Done.

+
+}  // namespace
 
 // Name of the directory in the user's profile directory where the notification
 // database files should be stored.
@@ -17,8 +24,15 @@ const base::FilePath::CharType kPlatformNotificationsDirectory[] =
     FILE_PATH_LITERAL("Platform Notifications");
 
 PlatformNotificationContextImpl::PlatformNotificationContextImpl(
-    const base::FilePath& path)
-    : path_(path) {
+    const base::FilePath& path,
+    const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context)
+    : path_(path),
+      service_worker_context_(service_worker_context) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PlatformNotificationContextImpl::InitializeOnIO, this));
 }
 
 PlatformNotificationContextImpl::~PlatformNotificationContextImpl() {
@@ -30,6 +44,30 @@ PlatformNotificationContextImpl::~PlatformNotificationContextImpl() {
   }
 }
 
+void PlatformNotificationContextImpl::InitializeOnIO() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // |service_worker_context_| may be NULL in tests.
+  if (service_worker_context_)
+    service_worker_context_->AddObserver(this);
+}
+
+void PlatformNotificationContextImpl::Shutdown() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  BrowserThread::PostTask(

[johnme, 2015/03/20 14:56:35]

My understanding is that there's now a race condition allowing the context to be
deleted on either the UI or IO threads. It seems this probably doesn't do any
harm (since you ensure NotificationDatabase is destroyed on the right thread),
but please add a comment* to the destructor documenting this, and please also
add BrowserThread::DeleteOnUIThread as the traits of
ServiceWorkerContextWrapper's RefCoountedThreadSafe superclass** (since
ServiceWorkerContextWrapper's ServiceWorkerProcessManager seems to assume it'll
always be deleted on the UI thread).

OR, more simply, add BrowserThread::DeleteOnUIThread as the traits of
PlatformNotificationContext's RefCountedThreadSafe superclass.

*: and/or a DCHECK:

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
BrowserThread::CurrentlyOn(BrowserThread::UI));

**: or (though this seems worse) reset service_worker_context_ in Shutdown().

[Peter Beverloo, 2015/03/20 18:39:05]

Good point.

I have moved the RefCountedThreadSafe<> to PlatformNotificationContextImpl and
removed it from the public API, and added the BrowserThread::DeleteOnUIThread to
the definition.

Additionally, a new DCHECK has appeared in the destructor. This means that we
won't release a reference to the ServiceWorkerContextWrapper on any other thread
than the UI thread anymore, matching the StoragePartitionImpl semantics. Their
destruction semantics are their concern.

+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PlatformNotificationContextImpl::ShutdownOnIO, this));
+}
+
+void PlatformNotificationContextImpl::ShutdownOnIO() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // |service_worker_context_| may be NULL in tests.
+  if (service_worker_context_)
+    service_worker_context_->RemoveObserver(this);
+}
+
 void PlatformNotificationContextImpl::ReadNotificationData(
     int64_t notification_id,
     const GURL& origin,
@@ -144,6 +182,31 @@ void PlatformNotificationContextImpl::DoDeleteNotificationData(
                           base::Bind(callback, success));
 }
 
+void PlatformNotificationContextImpl::OnRegistrationDeleted(
+    int64_t registration_id,
+    const GURL& pattern) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  LazyInitialize(
+      base::Bind(&PlatformNotificationContextImpl::
+                     DoDeleteNotificationsForServiceWorkerRegistration,
+                 this, pattern.GetOrigin(), registration_id),
+      base::Bind(&EmptyFailureCallback));
+}
+
+void PlatformNotificationContextImpl::
+    DoDeleteNotificationsForServiceWorkerRegistration(
+        const GURL& origin,
+        int64_t service_worker_registration_id) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  std::set<int64_t> deleted_notifications_set;
+  database_->DeleteAllNotificationDataForServiceWorkerRegistration(
+        origin, service_worker_registration_id, &deleted_notifications_set);
+
+  // TODO(peter): Record UMA on status for deleting from the database.

[johnme, 2015/03/20 14:56:35]

Nit: add a matching "Do the DeleteAndStartOver dance for
STATUS_ERROR_CORRUPTED." TODO?

[Peter Beverloo, 2015/03/20 18:39:05]

Done.

+  // TODO(peter): Close the notifications in |deleted_notifications_set|.
+}
+
 void PlatformNotificationContextImpl::LazyInitialize(
     const base::Closure& success_closure,
     const base::Closure& failure_closure) {