Delete the cookie DB when an unrecoverable error is detected.

chrome/browser/net/sqlite_persistent_cookie_store.cc

Index: chrome/browser/net/sqlite_persistent_cookie_store.cc
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.cc b/chrome/browser/net/sqlite_persistent_cookie_store.cc
index b519febee7cb315d43c5d277c6333abeb8449631..da9adcfea21fd7949e3369665daf1704f195b08f 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.cc
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.cc
@@ -32,6 +32,7 @@
 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
+#include "third_party/sqlite/sqlite3.h"
 
 using base::Time;
 using content::BrowserThread;
@@ -74,6 +75,8 @@ class SQLitePersistentCookieStore::Backend
         num_cookies_read_(0),
         num_priority_waiting_(0),
         total_priority_requests_(0) {
+    error_delegate_ =
+      new KillDatabaseErrorDelegate(this, GetErrorHandlerForCookieDb());
   }
 
   // Creates or loads the SQLite database.
@@ -104,6 +107,29 @@ class SQLitePersistentCookieStore::Backend
  private:
   friend class base::RefCountedThreadSafe<SQLitePersistentCookieStore::Backend>;
 
+  class KillDatabaseErrorDelegate : public sql::ErrorDelegate {
+   public:
+    KillDatabaseErrorDelegate(Backend* backend,
+                              sql::ErrorDelegate* wrapped_delegate);
+    // ErrorDelegate implementation.
+    virtual int OnError(int error,
+                        sql::Connection* connection,
+                        sql::Statement* stmt) OVERRIDE;
+
+    void reset_backend() {
+      backend_ = NULL;
+    }
+
+   private:
+    // Do not increment the count on Backend, as that would create a circular
+    // reference (Backend -> Connection -> ErrorDelegate -> Backend). Instead,
+    // Backend will call reset_backend() when it is going away.

[Scott Hess - ex-Googler, 2012/09/14 17:15:16]

Rather than doing this dance, would it make sense to have Backend simply
implement sql::ErrorDelegate directly?  Then you don't need to break the
backlink manually.

Alternately, I think if you reset the error delegate on the connection before
any call to db_.reset(), you'd never see inappropriate access.  Unless there are
thread-safety issues I'm missing.

[erikwright (departed), 2012/09/14 18:11:02]

Then you would have the circular reference of Backend -> Connection -> Backend,
and no way of breaking it.
I agree that there shouldn't be any inappropriate accesses if sql::Connection is
implemented the way one would expect.

Unforunately, we're passing around a reference counted object and there's no
explicit guarantee that it will be decremented instantly upon a
set_error_delegate(NULL).

reset_backend() exists purely for the sake of correctness, not to prevent any
actual crash.

[Scott Hess - ex-Googler, 2012/09/14 18:34:03]

Doh!
Hmm.  OK.  I honestly cannot see any reason for the error delegate to have been
ref-counted in the first place.

Part of why I'm nervous about manually breaking the link is because there are a
lot of places where db_ is reset().  Would it be reasonable to use a weak
pointer factory to generalize the solution, rather than adding manual
infrastructure?

[erikwright (departed), 2012/09/14 18:43:26]

We hold a pointer to the KillDatabaseErrorDelegate as a member of Backend. In
InternalBackgroundClose we break the back-ref before releasing our ref to the
delegate. In ~Backend() we DCHECK that Close has been called, though in the case
of db_ being reset you _could_ have a false positive there.

I have moved the reset_backend() to ~Backend, so it is now guaranteed that the
pointer to Backend is gone before Backend is destroyed.

+    Backend* backend_;
+    scoped_refptr<sql::ErrorDelegate> wrapped_delegate_;
+
+    DISALLOW_COPY_AND_ASSIGN(KillDatabaseErrorDelegate);
+  };
+
   // You should call Close() before destructing this object.
   ~Backend() {
     DCHECK(!db_.get()) << "Close should have already been called.";
@@ -189,8 +215,11 @@ class SQLitePersistentCookieStore::Backend
 
   void DeleteSessionCookiesOnShutdown();
 
+  void KillDatabase();
+
   FilePath path_;
   scoped_ptr<sql::Connection> db_;
+  scoped_refptr<KillDatabaseErrorDelegate> error_delegate_;
   sql::MetaTable meta_table_;
 
   typedef std::list<PendingOperation*> PendingOperationsList;
@@ -247,6 +276,99 @@ class SQLitePersistentCookieStore::Backend
   DISALLOW_COPY_AND_ASSIGN(Backend);
 };
 
+SQLitePersistentCookieStore::Backend::KillDatabaseErrorDelegate::
+KillDatabaseErrorDelegate(Backend* backend,
+                          sql::ErrorDelegate* wrapped_delegate)
+    : backend_(backend),
+      wrapped_delegate_(wrapped_delegate) {
+}
+
+int SQLitePersistentCookieStore::Backend::KillDatabaseErrorDelegate::OnError(
+    int error, sql::Connection* connection, sql::Statement* stmt) {
+  if (wrapped_delegate_.get())
+    error = wrapped_delegate_->OnError(error, connection, stmt);
+
+  bool delete_db = false;
+
+  switch (error) {
+    case SQLITE_DONE:
+    case SQLITE_OK:
+      // Theoretically, the wrapped delegate might have resolved the error, and
+      // we would end up here.
+      break;
+
+    case SQLITE_CORRUPT:
+    case SQLITE_NOTADB:
+      // Highly unlikely we would ever recover from these.
+      delete_db = true;
+      break;
+
+
+    case SQLITE_ERROR:

[Scott Hess - ex-Googler, 2012/09/14 17:01:34]

This an SQLITE_CONSTRAINT are _probably_ cases where we made a mistake.  Like we
did a schema upgrade incorrectly, or have incorrect SQLite code.  So killing the
database is likely to mask real problems in our code.

+      // TODO(erikwright): Figure out whether this is likely to be recoverable.
+      break;
+
+    case SQLITE_CANTOPEN:
+      // TODO(erikwright): Figure out what this means.
+      break;

[Scott Hess - ex-Googler, 2012/09/14 17:01:34]

Dammit, I wish I remember where I was seeing this, once.  I see a very old note
WRT Gears where SQLITE_CANTOPEN was associated with anti-virus checkers, but I
think there was something more recent.

+
+    case SQLITE_IOERR:
+    case SQLITE_BUSY:
+      // Presumably transient.
+      break;

[Scott Hess - ex-Googler, 2012/09/14 17:01:34]

It is possible to write the code such that SQLITE_BUSY is impossible.

SQLITE_IOERR is tricky.  If the file contains broken blocks, deleting it may
actually not be a bad idea.  But I'm not sure how often these kinds of things
are transient, like a write error.

[erikwright (departed), 2012/09/14 18:11:02]

SQLITE_IOERR is a relatively commonly occurring error. I'll try to dig into the
data to see if it tends to be transient or permanent.

+
+    case SQLITE_TOOBIG:
+    case SQLITE_FULL:
+    case SQLITE_NOMEM:
+      // Not a problem with the database.
+      break;
+
+    case SQLITE_READONLY:
+      // Presumably either transient or we don't have the privileges to
+      // move/delete the file anyway.
+      break;

[Scott Hess - ex-Googler, 2012/09/14 17:01:34]

We might be able to delete a read-only file.  Depending.  But you're right, this
is a wrongness which shouldn't be expected.

[erikwright (departed), 2012/09/14 18:11:02]

It is a commonly occuring error. Again, I'll dig into the data.

+
+    case SQLITE_CONSTRAINT:
+      // Presumably there is a schema upgrade failure.
+      // TODO(erikwright): Shall we kill the DB as an easy way out?
+      break;
+
+    case SQLITE_LOCKED:
+    case SQLITE_INTERNAL:
+    case SQLITE_PERM:
+    case SQLITE_ABORT:
+    case SQLITE_INTERRUPT:
+    case SQLITE_NOTFOUND:
+    case SQLITE_PROTOCOL:
+    case SQLITE_EMPTY:
+    case SQLITE_SCHEMA:
+    case SQLITE_MISMATCH:
+    case SQLITE_MISUSE:
+    case SQLITE_NOLFS:
+    case SQLITE_AUTH:
+    case SQLITE_FORMAT:
+    case SQLITE_RANGE:
+    case SQLITE_ROW:
+      // None of these appear in error reports, so for now let's not try to
+      // guess at how to handle them.
+      break;
+  }
+
+  if (delete_db && backend_) {
+    // Don't just do the close/delete here, as we are being called by |db| and
+    // that seems dangerous.
+    MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(&Backend::KillDatabase, backend_));
+
+    // Avoid being called more than once. There should still be a reference to
+    // this ErrorDelegate in the backend, but just in case don't refer to any
+    // members from here forward.
+    connection->set_error_delegate(wrapped_delegate_.get());
+  }
+
+  return error;
+}
+
 // Version number of the database.
 //
 // Version 5 adds the columns has_expires and is_persistent, so that the
@@ -494,14 +616,14 @@ bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
   }
 
   db_.reset(new sql::Connection);
+  db_->set_error_delegate(error_delegate_.get());
+
   if (!db_->Open(path_)) {
     NOTREACHED() << "Unable to open cookie DB.";
     db_.reset();
     return false;
   }
 
-  db_->set_error_delegate(GetErrorHandlerForCookieDb());
-
   if (!EnsureDatabaseVersion() || !InitTable(db_.get())) {
     NOTREACHED() << "Unable to open cookie DB.";
     db_.reset();
@@ -925,6 +1047,10 @@ void SQLitePersistentCookieStore::Backend::InternalBackgroundClose() {
   }
 
   db_.reset();
+  if (error_delegate_.get()) {
+    error_delegate_->reset_backend();
+    error_delegate_ = NULL;
+  }
 }
 
 void SQLitePersistentCookieStore::Backend::DeleteSessionCookiesOnShutdown() {
@@ -968,6 +1094,20 @@ void SQLitePersistentCookieStore::Backend::DeleteSessionCookiesOnShutdown() {
     LOG(WARNING) << "Unable to delete cookies on shutdown.";
 }
 
+void SQLitePersistentCookieStore::Backend::KillDatabase() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
+
+  if (db_.get()) {
+    // This Backend will now be in-memory only. In a future run we will recreate
+    // the database. Hopefully things go better then!
+    db_->Close();
+    db_.reset();
+  }
+
+  bool success = file_util::Delete(path_, false);

[Scott Hess - ex-Googler, 2012/09/14 17:01:34]

Suggest doing db_.Raze() before closing it.  That will clear the database down
to empty.

IF you still want the filesystem-level Delete, then you need to make sure you
delete the -journal file, and you might also want to delete a -wal file (which
isn't currently present, but which might be someday).  I cannot offhand make any
recommendations on how to order the deletes - I recall having an argument about
this on the SQLite mailing lists years ago, and Dr Hipp asserted that you cannot
really do a safe delete, which was a bit mind-boggling.

Actually, it would be much better to refactor to put the Delete() as a static
function in sql::Connection, so that anyone doing refactors around this can see
what's going on.

[erikwright (departed), 2012/09/14 18:11:02]

We can do the Raze for now and see what happens. We should see occurrences of
these errors drop dramatically, and we should presumably not see repeated
Cookie.KillDatabaseResult metrics from the same users.

+  UMA_HISTOGRAM_BOOLEAN("Cookie.KillDatabaseResult", success);
+}
+
 void SQLitePersistentCookieStore::Backend::SetForceKeepSessionState() {
   base::AutoLock locked(lock_);
   force_keep_session_state_ = true;