Switch to mojo localstorage backend by default.

content/browser/dom_storage/dom_storage_context_wrapper.cc

 // Copyright 2013 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/dom_storage_context_wrapper.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 86 matching lines @@
 
   context_ = new DOMStorageContextImpl(
       data_path.empty() ? data_path
                         : data_path.AppendASCII(kLocalStorageDirectory),
       data_path.empty() ? data_path
                         : data_path.AppendASCII(kSessionStorageDirectory),
       special_storage_policy,
       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);
     // TODO(mek): Use a SequencedTaskRunner once mojo no longer requires a
     // SingleThreadTaskRunner (http://crbug.com/678155).
     mojo_task_runner_ =
         BrowserThread::GetTaskRunnerForThread(BrowserThread::IO);
     mojo_state_ = new LocalStorageContextMojo(
         connector, context_->task_runner(),
         data_path.empty() ? data_path
@@ skipping 145 matching lines @@
     mojo_state_ = nullptr;
   }
   memory_pressure_listener_.reset();
   context_->task_runner()->PostShutdownBlockingTask(
       FROM_HERE,
       DOMStorageTaskRunner::PRIMARY_SEQUENCE,
       base::Bind(&DOMStorageContextImpl::Shutdown, context_));
   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() {
   DCHECK(context_.get());
 
   context_->task_runner()->PostShutdownBlockingTask(
       FROM_HERE, DOMStorageTaskRunner::PRIMARY_SEQUENCE,
       base::Bind(&DOMStorageContextImpl::Flush, context_));
   if (mojo_state_) {
     // base::Unretained is safe here, because the mojo_state_ won't be deleted
@@ skipping 14 matching lines @@
   // base::Unretained is safe here, because the mojo_state_ won't be deleted
   // until a ShutdownAndDelete task has been ran on the mojo_task_runner_, and
   // as soon as that task is posted, mojo_state_ is set to null, preventing
   // further tasks from being queued.
   mojo_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&LocalStorageContextMojo::OpenLocalStorage,
                                 base::Unretained(mojo_state_), origin,
                                 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 =
       DOMStorageContextImpl::PURGE_UNOPENED;
   if (memory_pressure_level ==
       base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) {
     purge_option = DOMStorageContextImpl::PURGE_AGGRESSIVE;
   }
   PurgeMemory(purge_option);
 }
@@ skipping 15 matching lines @@
     mojo_task_runner_->PostTask(
         FROM_HERE, base::BindOnce(&LocalStorageContextMojo::PurgeMemory,
                                   base::Unretained(mojo_state_)));
   }
 }
 
 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),
                  base::RetainedRef(reply_task_runner),
                  base::RetainedRef(context_), callback));
 }
 
 }  // namespace content