From 2bfde3811868b352d2d47bdfaa0564c7644d0218 Mon Sep 17 00:00:00 2001 From: "marja@chromium.org" Date: Wed, 18 Dec 2013 13:59:15 +0000 Subject: 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 --- content/browser/dom_storage/dom_storage_area.cc | 5 +- .../dom_storage/session_storage_database.cc | 61 +++++++++++++++++++--- .../browser/dom_storage/session_storage_database.h | 11 +++- 3 files changed, 68 insertions(+), 9 deletions(-) (limited to 'content') 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 areas; @@ -213,6 +261,7 @@ bool SessionStorageDatabase::ReadNamespacesAndOrigins( std::map >* 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; + class DBOperation; + friend class SessionStorageDatabase::DBOperation; friend class SessionStorageDatabaseTest; ~SessionStorageDatabase(); @@ -189,13 +191,20 @@ class CONTENT_EXPORT SessionStorageDatabase : scoped_ptr 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); }; -- cgit v1.1