Delete the cookie DB when an unrecoverable error is detected.

chrome/browser/net/sqlite_persistent_cookie_store.cc

 // Copyright (c) 2012 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 "chrome/browser/net/sqlite_persistent_cookie_store.h"
 
 #include <list>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 14 matching lines @@
 #include "base/time.h"
 #include "chrome/browser/diagnostics/sqlite_diagnostics.h"
 #include "chrome/browser/net/clear_on_exit_policy.h"
 #include "content/public/browser/browser_thread.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/canonical_cookie.h"
 #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;
 
 // This class is designed to be shared between any calling threads and the
 // database thread. It batches operations and commits them on a timer.
 //
 // SQLitePersistentCookieStore::Load is called to load all cookies.  It
 // delegates to Backend::Load, which posts a Backend::LoadAndNotifyOnDBThread
 // task to the DB thread.  This task calls Backend::ChainLoadCookies(), which
@@ skipping 22 matching lines @@
       : path_(path),
         db_(NULL),
         num_pending_(0),
         force_keep_session_state_(false),
         initialized_(false),
         restore_old_session_cookies_(restore_old_session_cookies),
         clear_on_exit_policy_(clear_on_exit_policy),
         num_cookies_read_(0),
         num_priority_waiting_(0),
         total_priority_requests_(0) {
+    error_delegate_ =
+      new KillDatabaseErrorDelegate(this, GetErrorHandlerForCookieDb());
   }
 
   // Creates or loads the SQLite database.
   void Load(const LoadedCallback& loaded_callback);
 
   // Loads cookies for the domain key (eTLD+1).
   void LoadCookiesForKey(const std::string& domain,
       const LoadedCallback& loaded_callback);
 
   // Batch a cookie addition.
@@ skipping 10 matching lines @@
 
   // Commit any pending operations and close the database.  This must be called
   // before the object is destructed.
   void Close();
 
   void SetForceKeepSessionState();
 
  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.";
     DCHECK(num_pending_ == 0 && pending_.empty());
   }
 
   // Database upgrade statements.
   bool EnsureDatabaseVersion();
 
   class PendingOperation {
@@ skipping 65 matching lines @@
                       const net::CanonicalCookie& cc);
   // Commit our pending operations to the database.
   void Commit();
   // Close() executed on the background thread.
   void InternalBackgroundClose();
 
   void DeleteSessionCookiesOnStartup();
 
   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;
   PendingOperationsList pending_;
   PendingOperationsList::size_type num_pending_;
   // True if the persistent store should skip delete on exit rules.
   bool force_keep_session_state_;
   // Guard |cookies_|, |pending_|, |num_pending_|, |force_keep_session_state_|
   base::Lock lock_;
 
@@ skipping 36 matching lines @@
   int total_priority_requests_;
   // The time when |num_priority_waiting_| incremented to 1.
   base::Time current_priority_wait_start_;
   // The cumulative duration of time when |num_priority_waiting_| was greater
   // than 1.
   base::TimeDelta priority_wait_duration_;
 
   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
 // database can store session cookies as well as persistent cookies. Databases
 // of version 5 are incompatible with older versions of code. If a database of
 // version 5 is read by older code, session cookies will be treated as normal
 // cookies. Currently, these fields are written, but not read anymore.
 //
 // In version 4, we migrated the time epoch.  If you open the DB with an older
 // version on Mac or Linux, the times will look wonky, but the file will likely
@@ skipping 227 matching lines @@
     return false;
   }
 
   int64 db_size = 0;
   if (file_util::GetFileSize(path_, &db_size)) {
     base::ThreadRestrictions::ScopedAllowIO allow_io;
     UMA_HISTOGRAM_COUNTS("Cookie.DBSizeInKB", db_size / 1024 );
   }
 
   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();
     return false;
   }
 
   UMA_HISTOGRAM_CUSTOM_TIMES(
     "Cookie.TimeInitializeDB",
     base::Time::Now() - start,
     base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromMinutes(1),
@@ skipping 403 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   // Commit any pending operations
   Commit();
 
   if (!force_keep_session_state_ && clear_on_exit_policy_.get() &&
       clear_on_exit_policy_->HasClearOnExitOrigins()) {
     DeleteSessionCookiesOnShutdown();
   }
 
   db_.reset();
+  if (error_delegate_.get()) {
+    error_delegate_->reset_backend();
+    error_delegate_ = NULL;
+  }
 }
 
 void SQLitePersistentCookieStore::Backend::DeleteSessionCookiesOnShutdown() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
 
   if (!db_.get())
     return;
 
   sql::Statement del_smt(db_->GetCachedStatement(
       SQL_FROM_HERE, "DELETE FROM cookies WHERE host_key=? AND secure=?"));
@@ skipping 23 matching lines @@
     del_smt.BindString(0, it->first.first);
     del_smt.BindInt(1, it->first.second);
     if (!del_smt.Run())
       NOTREACHED() << "Could not delete a cookie from the DB.";
   }
 
   if (!transaction.Commit())
     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;
 }
 
 void SQLitePersistentCookieStore::Backend::DeleteSessionCookiesOnStartup() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   if (!db_->Execute("DELETE FROM cookies WHERE persistent == 0"))
     LOG(WARNING) << "Unable to delete session cookies.";
 }
@@ skipping 45 matching lines @@
 }
 
 SQLitePersistentCookieStore::~SQLitePersistentCookieStore() {
   if (backend_.get()) {
     backend_->Close();
     // Release our reference, it will probably still have a reference if the
     // background thread has not run Close() yet.
     backend_ = NULL;
   }
 }