Destroy the notification database when corruption occurs.

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 030cb41e51455f5f6edd8aeccc56a0308eeee081..c8d132e31723bf80f438fc56ca65a2f8cab501aa 100644
--- a/content/browser/notifications/platform_notification_context_impl.cc
+++ b/content/browser/notifications/platform_notification_context_impl.cc
@@ -5,6 +5,7 @@
 #include "content/browser/notifications/platform_notification_context_impl.h"
 
 #include "base/bind_helpers.h"
+#include "base/files/file_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "content/browser/notifications/notification_database.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
@@ -88,6 +89,8 @@ void PlatformNotificationContextImpl::DoReadNotificationData(
                                       origin,
                                       &database_data);
 
+  // TODO(peter): Record UMA on |status| for reading from the database.
+
   if (status == NotificationDatabase::STATUS_OK) {
     BrowserThread::PostTask(BrowserThread::IO,
                             FROM_HERE,
@@ -97,8 +100,9 @@ void PlatformNotificationContextImpl::DoReadNotificationData(
     return;
   }
 
-  // TODO(peter): Record UMA on |status| for reading from the database.
-  // TODO(peter): Do the DeleteAndStartOver dance for STATUS_ERROR_CORRUPTED.
+  // Blow away the database if reading data failed due to corruption.
+  if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED)
+    DestroyDatabase();
 
   BrowserThread::PostTask(
       BrowserThread::IO,
@@ -129,9 +133,10 @@ void PlatformNotificationContextImpl::DoWriteNotificationData(
                                        database_data,
                                        &notification_id);
 
-  DCHECK_GT(notification_id, 0);
+  // TODO(peter): Record UMA on |status| for reading from the database.
 
   if (status == NotificationDatabase::STATUS_OK) {
+    DCHECK_GT(notification_id, 0);
     BrowserThread::PostTask(BrowserThread::IO,
                             FROM_HERE,
                             base::Bind(callback,
@@ -140,8 +145,9 @@ void PlatformNotificationContextImpl::DoWriteNotificationData(
     return;
   }
 
-  // TODO(peter): Record UMA on |status| for reading from the database.
-  // TODO(peter): Do the DeleteAndStartOver dance for STATUS_ERROR_CORRUPTED.
+  // Blow away the database if writing data failed due to corruption.
+  if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED)
+    DestroyDatabase();
 
   BrowserThread::PostTask(
       BrowserThread::IO,
@@ -169,10 +175,17 @@ void PlatformNotificationContextImpl::DoDeleteNotificationData(
   NotificationDatabase::Status status =
       database_->DeleteNotificationData(notification_id, origin);
 
-  const bool success = status == NotificationDatabase::STATUS_OK;
-
   // TODO(peter): Record UMA on |status| for reading from the database.
-  // TODO(peter): Do the DeleteAndStartOver dance for STATUS_ERROR_CORRUPTED.
+
+  bool success = status == NotificationDatabase::STATUS_OK;
+
+  // Blow away the database if reading data failed due to corruption. Following

[johnme, 2015/03/20 15:29:06]

s/reading/deleting/

[Peter Beverloo, 2015/03/20 18:55:37]

Done.

+  // 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();

[cmumford, 2015/03/20 17:36:52]

Well I guess you will have times where DestroyDatabase fails, but that will
almost surely leave a partial leveldb folder, so the notification data is likely
destroyed, so I guess calling this success is OK.

+    success = true;
+  }
 
   BrowserThread::PostTask(BrowserThread::IO,
                           FROM_HERE,
@@ -197,13 +210,26 @@ void PlatformNotificationContextImpl::
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   std::set<int64_t> deleted_notifications_set;
-  database_->DeleteAllNotificationDataForServiceWorkerRegistration(
-        origin, service_worker_registration_id, &deleted_notifications_set);
+  NotificationDatabase::Status status =
+      database_->DeleteAllNotificationDataForServiceWorkerRegistration(
+            origin, service_worker_registration_id, &deleted_notifications_set);
 
   // TODO(peter): Record UMA on status for deleting from the database.
+
+  // Blow away the database if a corruption error occurred during the deletion.
+  if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED)
+    DestroyDatabase();
+
   // TODO(peter): Close the notifications in |deleted_notifications_set|.
 }
 
+void PlatformNotificationContextImpl::OnStorageWiped() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  LazyInitialize(
+      base::Bind(&PlatformNotificationContextImpl::DestroyDatabase, this),
+      base::Bind(&DoNothing));
+}
+
 void PlatformNotificationContextImpl::LazyInitialize(
     const base::Closure& success_closure,
     const base::Closure& failure_closure) {
@@ -219,12 +245,14 @@ void PlatformNotificationContextImpl::LazyInitialize(
   task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&PlatformNotificationContextImpl::OpenDatabase,
-                 this, success_closure, failure_closure));
+                 this, success_closure, failure_closure,
+                 true /* delete_on_corruption */));
 }
 
 void PlatformNotificationContextImpl::OpenDatabase(
     const base::Closure& success_closure,
-    const base::Closure& failure_closure) {
+    const base::Closure& failure_closure,
+    bool delete_on_corruption) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   if (database_) {
@@ -233,24 +261,49 @@ void PlatformNotificationContextImpl::OpenDatabase(
   }
 
   database_.reset(new NotificationDatabase(GetDatabasePath()));
-
-  // TODO(peter): Record UMA on |status| for opening the database.
-  // TODO(peter): Do the DeleteAndStartOver dance for STATUS_ERROR_CORRUPTED.
-
   NotificationDatabase::Status status =
       database_->Open(true /* create_if_missing */);
 
+  // TODO(peter): Record UMA on |status| for opening the database.
+
   if (status == NotificationDatabase::STATUS_OK) {
     success_closure.Run();
     return;
   }
 
-  // TODO(peter): Properly handle failures when opening the database.
+  // When the database could not be opened due to corruption, and the
+  // |delete_on_corruption| parameter is true, blow it away and retry.
+  if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED &&
+      delete_on_corruption) {
+    DestroyDatabase();
+    OpenDatabase(success_closure,
+                 failure_closure,
+                 false /* delete_on_corruption */);

[cmumford, 2015/03/20 17:36:52]

I see your point about booleans - I prefer the relatively self-commenting nature
of enums myself.

What I was thinking is just to have
PlatformNotificationContextImpl::OpenDatabase do an open (if corrupt, delete the
folder (fail if that fails), and then open a second time. The caller (in this
simple example) wouldn't know that the db was wacked, but you could have a third
success_corruption_restored closure if you wanted that.

This way you could avoid recursion and have a simpler (at least to me) flow.

[Peter Beverloo, 2015/03/20 18:55:37]

That's a great suggestion - thank you once again! :-) It also allows us to track
the ability to open a database after it has been blown away due to corruption.

+    return;
+  }
+
   database_.reset();
 
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, failure_closure);
 }
 
+void PlatformNotificationContextImpl::DestroyDatabase() {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+  DCHECK(database_);
+
+  // TODO(peter): Record UMA on the status code of the Destroy() call.
+  database_->Destroy();
+  database_.reset();
+
+  // Remove all files in the directory that the database was previously located
+  // in, to make sure that any left-over files are gone as well.
+  base::FilePath database_path = GetDatabasePath();
+  if (!database_path.empty())
+    base::DeleteFile(database_path, true);
+
+  // TODO(peter): Close any existing persistent notifications on the platform.
+}
+
 base::FilePath PlatformNotificationContextImpl::GetDatabasePath() const {
   if (path_.empty())
     return path_;