Destroy the notification database when corruption occurs.

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/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"
 #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() {}
 
 }  // namespace
 
+// Defines the behavior to apply when a corrupt database is being opened.
+enum class PlatformNotificationContextImpl::CorruptionBehavior {
+  // Destroy the entire database and start over with an empty one.
+  DESTROY_AND_START_OVER,
+
+  // Abort the operation and invoke the failure callback.
+  FAIL_OPERATION,

[cmumford, 2015/03/19 22:12:27]

I can't really think of a time where you'd want to *not* delete a corrupted
database. The only thing that comes to mine are: 1) utility (diagnostic) apps,
or 2) working with a read-only filesystem.

If you don't currently intend to pass in FAIL_OPERATION then I'd just do away
with the enum and make the delete on corrupt db implicit.

[Peter Beverloo, 2015/03/20 14:31:26]

Done.

Note that this CL used this to avoid getting in an infinite loop when opening a
database that's corrupted. I've instead passed a boolean named
|delete_on_corruption|.

(I work a lot in Blink, where enums are preferred over booleans.)

+};
+
 // 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");
 
 PlatformNotificationContextImpl::PlatformNotificationContextImpl(
     const base::FilePath& path,
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context)
     : path_(path),
       service_worker_context_(service_worker_context) {
@@ skipping 53 matching lines @@
     const GURL& origin,
     const ReadResultCallback& callback) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   NotificationDatabaseData database_data;
   NotificationDatabase::Status status =
       database_->ReadNotificationData(notification_id,
                                       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,
                             base::Bind(callback,
                                        true /* success */,
                                        database_data));
     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,
       FROM_HERE,
       base::Bind(callback, false /* success */, NotificationDatabaseData()));
 }
 
 void PlatformNotificationContextImpl::WriteNotificationData(
     const GURL& origin,
     const NotificationDatabaseData& database_data,
@@ skipping 10 matching lines @@
     const NotificationDatabaseData& database_data,
     const WriteResultCallback& callback) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   int64_t notification_id = 0;
   NotificationDatabase::Status status =
       database_->WriteNotificationData(origin,
                                        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,
                                        true /* success */,
                                        notification_id));
     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,
       FROM_HERE,
       base::Bind(callback, false /* success */, 0 /* notification_id */));
 }
 
 void PlatformNotificationContextImpl::DeleteNotificationData(
     int64_t 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(
     int64_t notification_id,
     const GURL& origin,
     const DeleteResultCallback& callback) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   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): 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
+  // 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;
+  }
 
   BrowserThread::PostTask(BrowserThread::IO,
                           FROM_HERE,
                           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);
+  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(&EmptyFailureCallback));
+}
+
 void PlatformNotificationContextImpl::LazyInitialize(
     const base::Closure& success_closure,
     const base::Closure& failure_closure) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!task_runner_) {
     base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
     base::SequencedWorkerPool::SequenceToken token = pool->GetSequenceToken();
 
     task_runner_ = pool->GetSequencedTaskRunner(token);
   }
 
   task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&PlatformNotificationContextImpl::OpenDatabase,
-                 this, success_closure, failure_closure));
+                 this, success_closure, failure_closure,
+                 CorruptionBehavior::DESTROY_AND_START_OVER));
 }
 
 void PlatformNotificationContextImpl::OpenDatabase(
     const base::Closure& success_closure,
-    const base::Closure& failure_closure) {
+    const base::Closure& failure_closure,
+    CorruptionBehavior corruption_behavior) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   if (database_) {
     success_closure.Run();
     return;
   }
 
   database_.reset(new NotificationDatabase(GetDatabasePath()));
+  NotificationDatabase::Status status =
+      database_->Open(true /* create_if_missing */);
 
   // 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 */);
 
   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 corruption
+  // behavior is set to DESTROY_AND_START_OVER, blow away the database and retry
+  // opening it with the same success and failure callbacks.
+  if (status == NotificationDatabase::STATUS_ERROR_CORRUPTED &&
+      corruption_behavior == CorruptionBehavior::DESTROY_AND_START_OVER) {
+    DestroyDatabase();
+    OpenDatabase(success_closure,

[cmumford, 2015/03/19 22:12:27]

If I'm reading this correctly the user of this function can't know if the db was
deleted, and data was lost. I'll trust your judgement if that's OK.

[Peter Beverloo, 2015/03/20 14:31:26]

Indeed. The TODO on line 317 covers this - when the database gets destroyed, or
notifications get removed due to a Service Worker unregistering, the context
will take care of closing the necessary notifications with the
PlatformNotificationService.

This hasn't been implemented yet, hence the TODO.

+                 failure_closure,
+                 CorruptionBehavior::FAIL_OPERATION);
+    return;
+  }
+
   database_.reset();

[cmumford, 2015/03/19 22:12:27]

Do you want to reset this ptr above before calling OpenDatabase a second time? I
think it'll work fine, but might be clearer that way.

[Peter Beverloo, 2015/03/20 14:31:26]

Currently DestroyDatabase() calls NotificationDatabase::Destroy() on corruption
-- we only know whether corruption occurred after the database has been opened.
It then resets |database_| on line 309.

The reset on line 298 is for the "corruption or failure, but don't destroy the
database" case.

 
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, failure_closure);
 }
 
+void PlatformNotificationContextImpl::DestroyDatabase() {

[cmumford, 2015/03/19 22:12:27]

You should return a status code here, and check it above. You will have
antivirus software, etc. holding files open, preventing delete from succeeding,
and you'll want to catch that.

[Peter Beverloo, 2015/03/20 14:31:26]

That's an interesting case, I hadn't considered that. How should we track this?
UMA?

If we can't open the database, NotificationDatabase::Destroy() fails and
deleting the files fail as well, we'd end up invoking the failure closure in
OpenDatabase().

+  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_;
 
   return path_.Append(kPlatformNotificationsDirectory);
 }
 
 void PlatformNotificationContextImpl::SetTaskRunnerForTesting(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
   task_runner_ = task_runner;
 }
 
 }  // namespace content