SessionStorageDatabase: handle inconsistent and failed databases gracefully.

BUG=328144

Review URL: https://codereview.chromium.org/100623010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@241559 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 68 insertions, 9 deletions

diff --git a/content/browser/dom_storage/dom_storage_area.cc b/content/browser/dom_storage/dom_storage_area.cc
index e358301..b1e803a 100644
--- a/content/browser/dom_storage/dom_storage_area.cc
+++ b/content/browser/dom_storage/dom_storage_area.cc
@@ -361,9 +361,10 @@ void DOMStorageArea::OnCommitTimer() {
 void DOMStorageArea::CommitChanges(const CommitBatch* commit_batch) {
   // This method executes on the commit sequence.
   DCHECK(task_runner_->IsRunningOnCommitSequence());
-  bool success = backing_->CommitChanges(commit_batch->clear_all_first,
+  backing_->CommitChanges(commit_batch->clear_all_first,
                                          commit_batch->changed_values);
-  DCHECK(success);  // TODO(michaeln): what if it fails?
+  // TODO(michaeln): what if CommitChanges returns false (e.g., we're trying to
+  // commit to a DB which is in an inconsistent state?)
   task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&DOMStorageArea::OnCommitComplete, this));
diff --git a/content/browser/dom_storage/session_storage_database.cc b/content/browser/dom_storage/session_storage_database.cc
index 7b8dcd0..7fa6d5e 100644
--- a/content/browser/dom_storage/session_storage_database.cc
+++ b/content/browser/dom_storage/session_storage_database.cc
@@ -51,10 +51,53 @@ enum SessionStorageUMA {
 
 namespace content {
 
+// This class keeps track of ongoing operations across different threads. When
+// DB inconsistency is detected, we need to 1) make sure no new operations start
+// 2) wait until all current operations finish, and let the last one of them
+// close the DB and delete the data. The DB will remain empty for the rest of
+// the run, and will be recreated during the next run. We cannot hope to recover
+// during this run, since the upper layer will have a different idea about what
+// should be in the database.
+class SessionStorageDatabase::DBOperation {
+ public:
+  DBOperation(SessionStorageDatabase* session_storage_database)
+      : session_storage_database_(session_storage_database) {
+    base::AutoLock auto_lock(session_storage_database_->db_lock_);
+    ++session_storage_database_->operation_count_;
+  }
+
+  ~DBOperation() {
+    base::AutoLock auto_lock(session_storage_database_->db_lock_);
+    --session_storage_database_->operation_count_;
+    if ((session_storage_database_->is_inconsistent_ ||
+         session_storage_database_->db_error_) &&
+        session_storage_database_->operation_count_ == 0 &&
+        !session_storage_database_->invalid_db_deleted_) {
+      // No other operations are ongoing and the data is bad -> delete it now.
+      session_storage_database_->db_.reset();
+#if defined(OS_WIN)
+      leveldb::DestroyDB(
+          WideToUTF8(session_storage_database_->file_path_.value()),
+          leveldb::Options());
+#else
+      leveldb::DestroyDB(session_storage_database_->file_path_.value(),
+                         leveldb::Options());
+#endif
+      session_storage_database_->invalid_db_deleted_ = true;
+    }
+  }
+
+ private:
+  SessionStorageDatabase* session_storage_database_;
+};
+
+
 SessionStorageDatabase::SessionStorageDatabase(const base::FilePath& file_path)
     : file_path_(file_path),
       db_error_(false),
-      is_inconsistent_(false) {
+      is_inconsistent_(false),
+      invalid_db_deleted_(false),
+      operation_count_(0) {
 }
 
 SessionStorageDatabase::~SessionStorageDatabase() {
@@ -67,6 +110,7 @@ void SessionStorageDatabase::ReadAreaValues(const std::string& namespace_id,
   // nothing to be added to the result.
   if (!LazyOpen(false))
     return;
+  DBOperation operation(this);
 
   // While ReadAreaValues is in progress, another thread can call
   // CommitAreaChanges. CommitAreaChanges might update map ref count key while
@@ -92,6 +136,7 @@ bool SessionStorageDatabase::CommitAreaChanges(
   // in the database, so that it can be later shallow-copied succssfully.
   if (!LazyOpen(true))
     return false;
+  DBOperation operation(this);
 
   leveldb::WriteBatch batch;
   // Ensure that the keys "namespace-" "namespace-N" (see the schema above)
@@ -153,6 +198,7 @@ bool SessionStorageDatabase::CloneNamespace(
 
   if (!LazyOpen(true))
     return false;
+  DBOperation operation(this);
 
   leveldb::WriteBatch batch;
   const bool kOkIfExists = false;
@@ -181,6 +227,7 @@ bool SessionStorageDatabase::DeleteArea(const std::string& namespace_id,
     // No need to create the database if it doesn't exist.
     return true;
   }
+  DBOperation operation(this);
   leveldb::WriteBatch batch;
   if (!DeleteAreaHelper(namespace_id, origin.spec(), &batch))
     return false;
@@ -193,6 +240,7 @@ bool SessionStorageDatabase::DeleteNamespace(const std::string& namespace_id) {
     // No need to create the database if it doesn't exist.
     return true;
   }
+  DBOperation operation(this);
   // Itereate through the areas in the namespace.
   leveldb::WriteBatch batch;
   std::map<std::string, std::string> areas;
@@ -213,6 +261,7 @@ bool SessionStorageDatabase::ReadNamespacesAndOrigins(
     std::map<std::string, std::vector<GURL> >* namespaces_and_origins) {
   if (!LazyOpen(true))
     return false;
+  DBOperation operation(this);
 
   // While ReadNamespacesAndOrigins is in progress, another thread can call
   // CommitAreaChanges. To protect the reading operation, create a snapshot and
@@ -347,12 +396,11 @@ bool SessionStorageDatabase::ConsistencyCheck(bool ok) {
   if (ok)
     return true;
   base::AutoLock auto_lock(db_lock_);
-  DCHECK(false);
-  is_inconsistent_ = true;
   // We cannot recover the database during this run, e.g., the upper layer can
   // have a different understanding of the database state (shallow and deep
-  // copies).
-  // TODO(marja): Error handling.
+  // copies). Make further operations fail. The next operation that finishes
+  // will delete the data, and next run will recerate the database.
+  is_inconsistent_ = true;
   return false;
 }
 
@@ -360,8 +408,9 @@ bool SessionStorageDatabase::DatabaseErrorCheck(bool ok) {
   if (ok)
     return true;
   base::AutoLock auto_lock(db_lock_);
+  // Make further operations fail. The next operation that finishes
+  // will delete the data, and next run will recerate the database.
   db_error_ = true;
-  // TODO(marja): Error handling.
   return false;
 }
 
diff --git a/content/browser/dom_storage/session_storage_database.h b/content/browser/dom_storage/session_storage_database.h
index 09d4773..4260e4a 100644
--- a/content/browser/dom_storage/session_storage_database.h
+++ b/content/browser/dom_storage/session_storage_database.h
@@ -74,6 +74,8 @@ class CONTENT_EXPORT SessionStorageDatabase :
 
  private:
   friend class base::RefCountedThreadSafe<SessionStorageDatabase>;
+  class DBOperation;
+  friend class SessionStorageDatabase::DBOperation;
   friend class SessionStorageDatabaseTest;
 
   ~SessionStorageDatabase();
@@ -189,13 +191,20 @@ class CONTENT_EXPORT SessionStorageDatabase :
   scoped_ptr<leveldb::DB> db_;
   base::FilePath file_path_;
 
-  // For protecting the database opening code.
+  // For protecting the database opening code. Also guards the variables below.
   base::Lock db_lock_;
 
   // True if a database error has occurred (e.g., cannot read data).
   bool db_error_;
   // True if the database is in an inconsistent state.
   bool is_inconsistent_;
+  // True if the database is in a failed or inconsistent state, and we have
+  // already deleted it (as an attempt to recover later).
+  bool invalid_db_deleted_;
+
+  // The number of database operations in progress. We need this so that we can
+  // delete an inconsistent database at the right moment.
+  int operation_count_;
 
   DISALLOW_COPY_AND_ASSIGN(SessionStorageDatabase);
 };