Switch to mojo localstorage backend by default.

content/browser/dom_storage/dom_storage_context_wrapper.cc

Index: content/browser/dom_storage/dom_storage_context_wrapper.cc
diff --git a/content/browser/dom_storage/dom_storage_context_wrapper.cc b/content/browser/dom_storage/dom_storage_context_wrapper.cc
index 1a18095a5c5e08ee83efddfdefa2667a023070a6..7d0607d6fd9c1ff8f6c5fbbdcfa831af66e8db03 100644
--- a/content/browser/dom_storage/dom_storage_context_wrapper.cc
+++ b/content/browser/dom_storage/dom_storage_context_wrapper.cc
@@ -104,8 +104,8 @@ DOMStorageContextWrapper::DOMStorageContextWrapper(
       new DOMStorageWorkerPoolTaskRunner(std::move(primary_sequence),
                                          std::move(commit_sequence)));
 
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kMojoLocalStorage)) {
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDisableMojoLocalStorage)) {
     base::FilePath storage_dir;
     if (!profile_path.empty())
       storage_dir = local_partition_path.AppendASCII(kLocalStorageDirectory);
@@ -271,7 +271,7 @@ void DOMStorageContextWrapper::Shutdown() {
   if (base::FeatureList::IsEnabled(features::kMemoryCoordinator)) {
     base::MemoryCoordinatorClientRegistry::GetInstance()->Unregister(this);
   }
-
+  context_ = nullptr;

[michaeln, 2017/05/26 19:12:44]

this might turn up some crashes in odd cases where something unexpected calls us
after shutdown?

[Marijn Kruisselbrink, 2017/05/26 23:43:02]

attempting to post tasks to the TaskScheduler (as used by DOMTaskRunner) after
shutdown is already DCHECKing (the reason why I need to null this out in the
first place), so in debug builds this shouldn't change anything. If there is
code that is calling any DOMStorageContextWrapper methods after shutdown that's
a bug either way...

[michaeln, 2017/05/27 01:59:03]

calling contextwrapper methods after shutdown used to not be a bug, or at worst
an innocuous bug, but now they'll be crashing bugs

 }
 
 void DOMStorageContextWrapper::Flush() {
@@ -306,6 +306,13 @@ void DOMStorageContextWrapper::OpenLocalStorage(
                                 std::move(request)));
 }
 
+leveldb::mojom::LevelDBDatabaseAssociatedRequest
+DOMStorageContextWrapper::LocalStorageDatabaseRequestForTesting() {
+  if (!mojo_state_)
+    return nullptr;
+  return mojo_state_->DatabaseRequestForTesting();
+}
+
 void DOMStorageContextWrapper::OnMemoryPressure(
     base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level) {
   DOMStorageContextImpl::PurgeOption purge_option =
@@ -341,6 +348,9 @@ void DOMStorageContextWrapper::GotMojoLocalStorageUsage(
     GetLocalStorageUsageCallback callback,
     base::SingleThreadTaskRunner* reply_task_runner,
     std::vector<LocalStorageUsageInfo> usage) {
+  // If we were shutdown in the meantime, just give up.
+  if (!context_)

[michaeln, 2017/05/26 19:12:44]

we might need more early outs like this to protect agains UAF

alternatively, you could test mojo_state_ for an early out and not null out the
content_ ptr

wait, i think this is racy? the ptr value is cleared out on the UI thread, but
this method is running on the mojo_task_runner, so it could get nulled out
between the test on 352 and its use on 354

maybe use an AtomicFlag for is_shutdown_ and check that here instead?

[Marijn Kruisselbrink, 2017/05/26 23:43:02]

every other usage (other than the one in PurgeMemory, where I now added one)
already had DCHECKS for unclear reasons (since in the old code context_ could
never be null). Furthermore even without the DCHECK(context_) in all the other
methods, and without nulling out the context_ in Shutdown, every other of those
usages would likely DCHECK inside TaskScheduler code anyway after shutdown
(which is the reason I'm nulling out context_ during Shutdown to begin with.
Without it context_ might survive till after shutdown if some unrelated piece of
code is holding on to a rerference to DOMStorageContextWrapper, at which point
destruction of it post-shutdown will DCHECK because ~DOMStorageContextImpl tries
to post a task).
Fixed the raciness by changing how GetLocalStorageUsage combines the data from
the two sources (in the new code it just runs both at the same time, combining
the results on the main thread and passing the combined data back to the
callback when both have finished).

But anyway, any kind of is_shutdown flag or testing something else wouldn't
really help, since the fundamental problem is with DOMStorageContextImpl living
on TaskScheduler managed threads, and it trying to post tasks to those in its
destructor. Which together means DOMStorageContextImpl has to be destroyed
before shutdown is complete (modulo task scheduler managed shutdown blocking
tasks still being around).

[michaeln, 2017/05/27 01:59:03]

The dchecks were to document it's non-nullness and to make it clear if you had
an instance pointer, methods were safe to call... which is no longer true.

If the problem is the taskposting in the dtor, another more focused fix for that
problem could be to post the SessionStorageDatabase::Release task from within
the Shutdown() method instead of the dtor. We wouldn't need to null out the
context_ ptr in that case and methods of this class would continue to be safe to
call up until TaskScheduler::Shutdown completion.

wdyt? i'm a little concerned about introducing new crashing shutdown bugs
that's a lot nicer!

[Marijn Kruisselbrink, 2017/05/30 20:53:55]

I agree that the current CL is somewhat scare in that regard, yes... Not sure if
nulling out the SessionStorageDatabase in DOMStorageContextImpl::Shutdown would
necessarily better though (there wouldn't be a need to PostTask, as Shutdown is
already being executed on the correct thread). Nulling out
session_storage_database_ before destruction would also require destroying
several other members of DOMStorageContextImpl (that hold raw pointers to
session_storage_database_).
At least most/all code in DOMStorageContextImpl already seems to guard access to
session_storage_database_ with an if(session_storage_database_) or a if
(!is_shutdown_) so hopefully the risk here would at least be smaller than the
risk of nulling out context_...

+    return;
   context_->task_runner()->PostShutdownBlockingTask(
       FROM_HERE, DOMStorageTaskRunner::PRIMARY_SEQUENCE,
       base::Bind(&GetLocalStorageUsageHelper, base::Passed(&usage),