Delete and try to recreate localstorage database on invalid schema version.

content/browser/dom_storage/local_storage_context_mojo.cc

Index: content/browser/dom_storage/local_storage_context_mojo.cc
diff --git a/content/browser/dom_storage/local_storage_context_mojo.cc b/content/browser/dom_storage/local_storage_context_mojo.cc
index 01d0c82eef583922947de48d079a797e777246b3..6e46fb085ea2bfe816562e5b3d722deea09c4f22 100644
--- a/content/browser/dom_storage/local_storage_context_mojo.cc
+++ b/content/browser/dom_storage/local_storage_context_mojo.cc
@@ -50,7 +50,8 @@ std::vector<uint8_t> CreateMetaDataKey(const url::Origin& origin) {
 }
 
 void NoOpSuccess(bool success) {}
-}
+
+}  // namespace
 
 LocalStorageContextMojo::LocalStorageContextMojo(
     service_manager::Connector* connector,
@@ -123,22 +124,7 @@ void LocalStorageContextMojo::RunWhenConnected(base::OnceClosure callback) {
         base::Bind(&LocalStorageContextMojo::OnUserServiceConnectionError,
                    weak_ptr_factory_.GetWeakPtr()));
 
-    if (!subdirectory_.empty()) {
-      // We were given a subdirectory to write to. Get it and use a disk backed
-      // database.
-      file_service_connection_->GetInterface(&file_system_);
-      file_system_->GetSubDirectory(
-          subdirectory_.AsUTF8Unsafe(), MakeRequest(&directory_),
-          base::Bind(&LocalStorageContextMojo::OnDirectoryOpened,
-                     weak_ptr_factory_.GetWeakPtr()));
-    } else {
-      // We were not given a subdirectory. Use a memory backed database.
-      file_service_connection_->GetInterface(&leveldb_service_);
-      leveldb_service_->OpenInMemory(
-          MakeRequest(&database_),
-          base::Bind(&LocalStorageContextMojo::OnDatabaseOpened,
-                     weak_ptr_factory_.GetWeakPtr()));
-    }
+    InitiateConnection();
   }
 
   if (connection_state_ == CONNECTION_IN_PROGRESS) {
@@ -204,7 +190,26 @@ void LocalStorageContextMojo::OnUserServiceConnectionError() {
   CHECK(false);
 }
 
-// Part of our asynchronous directory opening called from OpenLocalStorage().
+void LocalStorageContextMojo::InitiateConnection(bool in_memory_only) {
+  DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
+  if (!subdirectory_.empty() && !in_memory_only) {
+    // We were given a subdirectory to write to. Get it and use a disk backed
+    // database.
+    file_service_connection_->GetInterface(&file_system_);
+    file_system_->GetSubDirectory(
+        subdirectory_.AsUTF8Unsafe(), MakeRequest(&directory_),
+        base::Bind(&LocalStorageContextMojo::OnDirectoryOpened,
+                   weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    // We were not given a subdirectory. Use a memory backed database.
+    file_service_connection_->GetInterface(&leveldb_service_);
+    leveldb_service_->OpenInMemory(
+        MakeRequest(&database_),
+        base::Bind(&LocalStorageContextMojo::OnDatabaseOpened,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
 void LocalStorageContextMojo::OnDirectoryOpened(
     filesystem::mojom::FileError err) {
   if (err != filesystem::mojom::FileError::OK) {
@@ -218,10 +223,14 @@ void LocalStorageContextMojo::OnDirectoryOpened(
   // database.
   file_service_connection_->GetInterface(&leveldb_service_);
 
+  // We might still need to directory, so create a clone.
+  filesystem::mojom::DirectoryPtr directory_clone;
+  directory_->Clone(MakeRequest(&directory_clone));
+
   auto options = leveldb::mojom::OpenOptions::New();
   options->create_if_missing = true;
   leveldb_service_->OpenWithOptions(
-      std::move(options), std::move(directory_), "leveldb",
+      std::move(options), std::move(directory_clone), "leveldb",
       MakeRequest(&database_),
       base::Bind(&LocalStorageContextMojo::OnDatabaseOpened,
                  weak_ptr_factory_.GetWeakPtr()));
@@ -233,14 +242,8 @@ void LocalStorageContextMojo::OnDatabaseOpened(
     // If we failed to open the database, reset the service object so we pass
     // null pointers to our wrappers.
     database_.reset();
-    leveldb_service_.reset();
   }
 
-  // We no longer need the file service; we've either transferred |directory_|
-  // to the leveldb service, or we got a file error and no more is possible.
-  directory_.reset();
-  file_system_.reset();
-
   // Verify DB schema version.
   if (database_) {
     database_->Get(leveldb::StdStringToUint8Vector(kVersionKey),
@@ -266,17 +269,30 @@ void LocalStorageContextMojo::OnGotDatabaseVersion(
     if (!base::StringToInt64(leveldb::Uint8VectorToStdString(value),
                              &db_version) ||
         db_version < kMinSchemaVersion || db_version > kCurrentSchemaVersion) {
-      // TODO(mek): delete and recreate database, rather than failing outright.
-      database_ = nullptr;
+      DeleteAndRecreateDatabase();
+      return;
     }
 
     database_initialized_ = true;
   } else {
     // Other read error. Possibly database corruption.
-    // TODO(mek): delete and recreate database, rather than failing outright.
-    database_ = nullptr;
+    DeleteAndRecreateDatabase();
+    return;
   }
 
+  OnConnectionFinished();
+}
+
+void LocalStorageContextMojo::OnConnectionFinished() {
+  DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
+
+  // We no longer need the file service; we've either transferred |directory_|
+  // to the leveldb service, or we got a file error and no more is possible.
+  directory_.reset();
+  file_system_.reset();
+  if (!database_)
+    leveldb_service_.reset();
+
   // |database_| should be known to either be valid or invalid by now. Run our
   // delayed bindings.
   connection_state_ = CONNECTION_FINISHED;
@@ -285,6 +301,56 @@ void LocalStorageContextMojo::OnGotDatabaseVersion(
   on_database_opened_callbacks_.clear();
 }
 
+void LocalStorageContextMojo::DeleteAndRecreateDatabase() {
+  // For now don't support deletion and recreation when already connected.
+  DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
+
+  bool recreate_in_memory = false;
+
+  // If tried to recreate database on disk already, try again but this time
+  // in memory.
+  if (tried_to_recreate_ && !subdirectory_.empty()) {
+    recreate_in_memory = true;
+  } else if (tried_to_recreate_) {
+    // Give up completely, run without any database.
+    database_ = nullptr;
+    OnConnectionFinished();
+    return;
+  }
+
+  tried_to_recreate_ = true;
+
+  // Unit tests might not have a file_service_connection_, in which case there
+  // is nothing to rety.
+  if (!file_service_connection_) {
+    database_ = nullptr;
+    OnConnectionFinished();
+    return;
+  }
+
+  // Close and destroy database, and try again.
+  // TODO(mek): Somehow ensure closing is processed before Destroy is called.

[Marijn Kruisselbrink, 2017/01/11 19:10:58]

This TODO is somewhat problematic, since I'm not sure how to accomplish such a
thing. Maybe the Destroy call should be part of the LevelDBDatabase interface
rather than LevelDBService (where it could make sure to close itself before
destroying itself), but then you'd lose the ability to destroy a database
without opening it first. Anything else seems like it will have the problem that
the leveldb service and leveldb database are separate mojo pipes and thus
relying on something on one pipe to have finished before calling a method on the
other pipe is processed is dangerous. Any thoughts?

[jam, 2017/01/12 17:24:42]

If Destroy is on LevelDBInterface, won't there be the same issue for ordering
since the Open is on LevelDBService?

To solve the ordering issue, we could use associated interfaces. wdyt?
https://www.chromium.org/developers/design-documents/mojo/associated-interfaces

[Marijn Kruisselbrink, 2017/01/13 19:25:11]

I suppose so, although it seemed less likely that you'd be trying to open a
database while simultaneously trying to destroy it than that you'd want to
destroy a database that you had open already.
Yes, that would work. Changed it to do so. Somehow using associated interface
does kind of always feel like doing it the wrong way to me, with it being a c++
only feature and all (or did that change recently?)...

[jam, 2017/01/14 00:17:55]

JavaScript support is being worked on :)

Associated interfaces shouldn't be viewed negatively, it's very useful to split
logically separate methods into separate interfaces, while still maintaining
order. Otherwise things that don't belong together are grouped.

+  database_ = nullptr;
+  if (directory_.is_bound()) {
+    leveldb_service_->Destroy(
+        std::move(directory_), "leveldb",
+        base::Bind(&LocalStorageContextMojo::OnDBDestroyed,
+                   weak_ptr_factory_.GetWeakPtr(), recreate_in_memory));
+  } else {
+    // No directory, so nothing to destroy. Retrying to recreate will probably
+    // fail, but try anyway.
+    InitiateConnection(recreate_in_memory);
+  }
+}
+
+void LocalStorageContextMojo::OnDBDestroyed(
+    bool recreate_in_memory,
+    leveldb::mojom::DatabaseError status) {
+  // We're essentially ignoring the status here. Even if destroying failed we
+  // still want to go ahead and try to recreate.
+  InitiateConnection(recreate_in_memory);
+}
+
 // The (possibly delayed) implementation of OpenLocalStorage(). Can be called
 // directly from that function, or through |on_database_open_callbacks_|.
 void LocalStorageContextMojo::BindLocalStorage(