Add database recovery for FileSystemOriginDatabase

webkit/fileapi/file_system_origin_database.cc

Index: webkit/fileapi/file_system_origin_database.cc
diff --git a/webkit/fileapi/file_system_origin_database.cc b/webkit/fileapi/file_system_origin_database.cc
index 125576ae38d1eda4f7dfeae35a63059f1b326c06..15ff24f895db0dfe5921574242c492fad31e1fd5 100644
--- a/webkit/fileapi/file_system_origin_database.cc
+++ b/webkit/fileapi/file_system_origin_database.cc
@@ -4,6 +4,7 @@
 
 #include "webkit/fileapi/file_system_origin_database.h"
 
+#include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -16,6 +17,7 @@
 
 namespace {
 
+const FilePath::CharType kOriginDatabaseName[] = FILE_PATH_LITERAL("Origins");
 const char kOriginKeyPrefix[] = "ORIGIN:";
 const char kLastPathKey[] = "LAST_PATH";
 
@@ -43,31 +45,105 @@ FileSystemOriginDatabase::OriginRecord::OriginRecord(
 FileSystemOriginDatabase::OriginRecord::~OriginRecord() {
 }
 
-FileSystemOriginDatabase::FileSystemOriginDatabase(const FilePath& path) {
-#if defined(OS_POSIX)
-  path_ = path.value();
-#elif defined(OS_WIN)
-  path_ = base::SysWideToUTF8(path.value());
-#endif
+FileSystemOriginDatabase::FileSystemOriginDatabase(
+    const FilePath& file_system_directory)
+    : file_system_directory_(file_system_directory) {
 }
 
 FileSystemOriginDatabase::~FileSystemOriginDatabase() {
 }
 
-bool FileSystemOriginDatabase::Init() {
+bool FileSystemOriginDatabase::Init(RecoveryOption recovery_option) {
   if (db_.get())
     return true;
 
+  std::string path;
+#if defined(OS_POSIX)
+  path = file_system_directory_.Append(kOriginDatabaseName).value();
+#elif defined(OS_WIN)
+  path = base::SysWideToUTF8(
+      file_system_directory_.Append(kOriginDatabaseName).value());
+#else
+  NOTREACHED();
+#endif

[kinuko, 2012/03/23 04:01:33]

Can we use AsUTF8Unsafe and remove ifdefs? (Here and elsewhere)

[tzik, 2012/03/23 07:09:00]

ditto

+
   leveldb::Options options;
   options.create_if_missing = true;
   leveldb::DB* db;
-  leveldb::Status status = leveldb::DB::Open(options, path_, &db);
+  leveldb::Status status = leveldb::DB::Open(options, path, &db);
+  // TODO(tzik): Collect status metrics here.
   if (status.ok()) {
     db_.reset(db);
     return true;
   }
   HandleError(FROM_HERE, status);
-  return false;
+
+  if (recovery_option == FAIL_ON_CORRUPTION)
+    return false;
+
+  if (recovery_option == REPAIR_ON_CORRUPTION && RepairDB(path))
+    return true;
+
+  DCHECK(DELETE_ON_CORRUPTION == recovery_option ||
+         REPAIR_ON_CORRUPTION == recovery_option);
+  if (!file_util::Delete(file_system_directory_, true))
+    return false;
+  if (!file_util::CreateDirectory(file_system_directory_))
+    return false;
+
+  return Init(FAIL_ON_CORRUPTION);
+}
+
+bool FileSystemOriginDatabase::RepairDB(const std::string& db_path) {
+  DCHECK(!db_.get());
+  if (!leveldb::RepairDB(db_path, leveldb::Options()).ok())
+    return false;
+  if (!Init(FAIL_ON_CORRUPTION))
+    return false;

[kinuko, 2012/03/23 04:01:33]

If we think we shouldn't fail here (or somewhere) after the repair maybe we
should add a WARNING log before returning false?

[tzik, 2012/03/23 07:09:00]

Done.

+

[kinuko, 2012/03/23 04:01:33]

Can we add a brief comment to state what we're doing here:
// See if the repaired entries match with what we have on disk.

[tzik, 2012/03/23 07:09:00]

Done.

+  std::set<FilePath> directories;
+  file_util::FileEnumerator file_enum(file_system_directory_,
+                                      false /* recursive */,
+                                      file_util::FileEnumerator::DIRECTORIES);
+  FilePath path_each;
+  while (!(path_each = file_enum.Next()).empty())
+    directories.insert(path_each.BaseName());

[kinuko, 2012/03/23 04:01:33]

Maybe we could skip inserting path here if it is kOriginDaatbaseName?

[tzik, 2012/03/23 07:09:00]

I'd like to leave it if it won't hurt anything.
The DCHECK() below prevents me to "repair" completely wrong directory.

[kinuko, 2012/03/26 05:25:09]

Ok.  Might be worth adding a short comment for the DCHECK like: 'Make sure we
have the database file in its directory and therefore we are working on the
correct path.'?

[tzik, 2012/03/26 08:04:56]

Done.

+  std::set<FilePath>::iterator db_dir_itr =
+      directories.find(FilePath(kOriginDatabaseName));
+  DCHECK(db_dir_itr != directories.end());
+  directories.erase(db_dir_itr);
+
+  std::vector<OriginRecord> origins;
+  if (!ListAllOrigins(&origins)) {
+    DropDatabase();
+    return false;
+  }
+  for (std::vector<OriginRecord>::iterator db_origin_itr = origins.begin();
+       db_origin_itr != origins.end();
+       ++db_origin_itr) {
+    std::set<FilePath>::iterator dir_itr =
+        directories.find(db_origin_itr->path);
+    if (dir_itr == directories.end()) {
+      if (!RemovePathForOrigin(db_origin_itr->origin)) {
+        DropDatabase();
+        return false;
+      }
+    } else {
+      directories.erase(dir_itr);
+    }
+  }
+
+  // Delete any directories not listed in the origins database.
+  for (std::set<FilePath>::iterator dir_itr = directories.begin();
+       dir_itr != directories.end();
+       ++dir_itr)
+    if (!file_util::Delete(file_system_directory_.Append(*dir_itr),
+                           true /* recursive */)) {
+      DropDatabase();
+      return false;
+    }

[kinuko, 2012/03/23 04:01:33]

Please put { } around for body if it spans multiple lines

[tzik, 2012/03/23 07:09:00]

Done.

+
+  return true;
 }
 
 void FileSystemOriginDatabase::HandleError(
@@ -78,7 +154,7 @@ void FileSystemOriginDatabase::HandleError(
 }
 
 bool FileSystemOriginDatabase::HasOriginPath(const std::string& origin) {
-  if (!Init())
+  if (!Init(REPAIR_ON_CORRUPTION))
     return false;
   if (origin.empty())
     return false;
@@ -95,7 +171,7 @@ bool FileSystemOriginDatabase::HasOriginPath(const std::string& origin) {
 
 bool FileSystemOriginDatabase::GetPathForOrigin(
     const std::string& origin, FilePath* directory) {
-  if (!Init())
+  if (!Init(REPAIR_ON_CORRUPTION))
     return false;
   DCHECK(directory);
   if (origin.empty())
@@ -132,7 +208,7 @@ bool FileSystemOriginDatabase::GetPathForOrigin(
 }
 
 bool FileSystemOriginDatabase::RemovePathForOrigin(const std::string& origin) {
-  if (!Init())
+  if (!Init(REPAIR_ON_CORRUPTION))
     return false;
   leveldb::Status status =
       db_->Delete(leveldb::WriteOptions(), OriginToOriginKey(origin));
@@ -144,7 +220,7 @@ bool FileSystemOriginDatabase::RemovePathForOrigin(const std::string& origin) {
 
 bool FileSystemOriginDatabase::ListAllOrigins(
     std::vector<OriginRecord>* origins) {
-  if (!Init())
+  if (!Init(REPAIR_ON_CORRUPTION))
     return false;
   DCHECK(origins);
   scoped_ptr<leveldb::Iterator> iter(db_->NewIterator(leveldb::ReadOptions()));
@@ -171,7 +247,7 @@ void FileSystemOriginDatabase::DropDatabase() {
 }
 
 bool FileSystemOriginDatabase::GetLastPathNumber(int* number) {
-  if (!Init())
+  if (!Init(REPAIR_ON_CORRUPTION))
     return false;
   DCHECK(number);
   std::string number_string;