Switch to mojo localstorage backend by default.

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 2be6213f8ce2f40c4be1d0821cb30765217a20f4..3d9c807c9b628dcd5b6419dd386eba893ec49911 100644
--- a/content/browser/dom_storage/local_storage_context_mojo.cc
+++ b/content/browser/dom_storage/local_storage_context_mojo.cc
@@ -341,14 +341,12 @@ void LocalStorageContextMojo::PurgeMemory() {
     it.second->level_db_wrapper()->PurgeMemory();
 }
 
-leveldb::mojom::LevelDBDatabaseAssociatedRequest
-LocalStorageContextMojo::DatabaseRequestForTesting() {
+void LocalStorageContextMojo::SetDatabaseForTesting(
+    leveldb::mojom::LevelDBDatabaseAssociatedPtr database) {
   DCHECK_EQ(connection_state_, NO_CONNECTION);
   connection_state_ = CONNECTION_IN_PROGRESS;
-  leveldb::mojom::LevelDBDatabaseAssociatedRequest request =
-      MakeIsolatedRequest(&database_);
+  database_ = std::move(database);
   OnDatabaseOpened(true, leveldb::mojom::DatabaseError::OK);
-  return request;
 }
 
 // static
@@ -388,7 +386,14 @@ void LocalStorageContextMojo::RunWhenConnected(base::OnceClosure callback) {
 
 void LocalStorageContextMojo::InitiateConnection(bool in_memory_only) {
   DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
-  CHECK(connector_);
+
+  // Unit tests might not always have a Connector, use in-memory only if that
+  // happens.

[dcheng, 2017/06/06 22:22:55]

I think I've mentioned it elsewhere but it'd be nice if it was easy (possible?)
for unit tests to set this up so we didn't have otherwise pointless branches in
production code. Is there some long-term plan to address these sort of issues?

(This is partially from personal experience: I've definitely hit issues where I
was unexpectedly hitting a "this should only be null for testing"... except it
wasn't a test. It was quite frustrating.)

[Marijn Kruisselbrink, 2017/06/06 22:52:35]

It is definitely possible to do so. All of the LocalStorage specific unit tests
that hit this codepath do so by deriving from
service_manager::test::ServiceTest.

Where this became an issue was in a bunch of Extension unit tests (see
https://luci-milo.appspot.com/buildbot/tryserver.chromium.linux/linux_chromiu...)
where uninstalling an extension ends up trying to clear storage for the
extensions origin. So at some point between StoragePartitionImpl::Create and
here something needs to be skipped if BrowserContext::GetConnectorFor returns
null, and I'm not sure where the best place is (if not here). Of course every
place has the same issue of adding test only code paths to production code.

Ultimately the correct fix is probably for the extensions unit tests to not rely
on vast amounts of the rest of the system to be setup, mocking out more of the
StoragePartition/Profile etc... There really is no reason that extension unit
tests should end up needing an actual localstorage implementation...

[dcheng, 2017/06/06 23:02:44]

Meh. I'll turn my eye the other way and pretend this doesn't exist then. Maybe
file a bug for the extensions team so we can at least track the (hypothetical)
cleanup here.

+  if (!connector_) {
+    OnDatabaseOpened(false, leveldb::mojom::DatabaseError::OK);
+    return;
+  }
+
   if (!subdirectory_.empty() && !in_memory_only) {
     // We were given a subdirectory to write to. Get it and use a disk backed
     // database.
@@ -607,6 +612,13 @@ LevelDBWrapperImpl* LocalStorageContextMojo::GetOrCreateDBWrapper(
 
 void LocalStorageContextMojo::RetrieveStorageUsage(
     GetStorageUsageCallback callback) {
+  if (!database_) {
+    // If for whatever reason no leveldb database is available, no storage is
+    // used, so return an empty array.

[dcheng, 2017/06/06 22:22:55]

Does this mean the renderer did something bad?

[Marijn Kruisselbrink, 2017/06/06 22:52:35]

No, this is not related to connections to the renderer at all. This is about the
connection from the localstorage code in the browser process to the leveldb
service (in the browser process). So database_ would be null if we failed to
connect to the leveldb service (for example the unit-test only situation of not
having a Connector...), or opening the database itself failed (twice, if the
first time fails, we try to throw away all files on disk and try opening it
again).

+    std::move(callback).Run(std::vector<LocalStorageUsageInfo>());
+    return;
+  }
+
   database_->GetPrefixed(
       std::vector<uint8_t>(kMetaPrefix, kMetaPrefix + arraysize(kMetaPrefix)),
       base::Bind(&LocalStorageContextMojo::OnGotMetaData,