Switch to mojo localstorage backend by default.

content/browser/dom_storage/local_storage_context_mojo.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/dom_storage/local_storage_context_mojo.h"
 
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 323 matching lines @@
   } else {
     OnShutdownComplete(leveldb::mojom::DatabaseError::OK);
   }
 }
 
 void LocalStorageContextMojo::PurgeMemory() {
   for (const auto& it : level_db_wrappers_)
     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
 std::vector<uint8_t> LocalStorageContextMojo::MigrateString(
     const base::string16& input) {
   static const uint8_t kUTF16Format = 0;
 
   const uint8_t* data = reinterpret_cast<const uint8_t*>(input.data());
   std::vector<uint8_t> result;
   result.reserve(input.size() * sizeof(base::char16) + 1);
@@ skipping 19 matching lines @@
     // Queue this OpenLocalStorage call for when we have a level db pointer.
     on_database_opened_callbacks_.push_back(std::move(callback));
     return;
   }
 
   std::move(callback).Run();
 }
 
 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.
     connector_->BindInterface(file::mojom::kServiceName, &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.
@@ skipping 198 matching lines @@
     return found->second->level_db_wrapper();
 
   auto holder = base::MakeUnique<LevelDBWrapperHolder>(this, origin);
   LevelDBWrapperImpl* wrapper_ptr = holder->level_db_wrapper();
   level_db_wrappers_[origin] = std::move(holder);
   return wrapper_ptr;
 }
 
 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,
                  weak_ptr_factory_.GetWeakPtr(), base::Passed(&callback)));
 }
 
 void LocalStorageContextMojo::OnGotMetaData(
     GetStorageUsageCallback callback,
     leveldb::mojom::DatabaseError status,
     std::vector<leveldb::mojom::KeyValuePtr> data) {
@@ skipping 52 matching lines @@
     OnShutdownComplete(leveldb::mojom::DatabaseError::OK);
   }
 }
 
 void LocalStorageContextMojo::OnShutdownComplete(
     leveldb::mojom::DatabaseError error) {
   delete this;
 }
 
 }  // namespace content