Switch to mojo localstorage backend by default.

Switch to mojo localstorage backend by default.

Old non-mojo sqlite based backend is still available with
--disable-mojo-local-storage. The mojo-localstorage virtual layout test
suite is switched to now test the non-mojo local storage implementation.

Other than changing the default, some minor changes are made to make
sure all existing tests still pass:
- LocalStorageContextMojo is made to work in unit tests where no Connector
  exists
- Several StoragePartitionImpl unittests relied on localstorage internals,
  these tests were updated to now work with the new leveldb implementation.

BUG=586194
Review-Url: https://codereview.chromium.org/2847013002
Cr-Commit-Position: refs/heads/master@{#477458}
Committed: https://chromium.googlesource.com/chromium/src/+/c8e2b27f71c6792fa1072fa98a22c111ad229090

[Marijn Kruisselbrink, 2017-04-28 16:37:58]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 16:38:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 17:08:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 17:08:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Marijn Kruisselbrink, 2017-04-28 18:02:13]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 18:04:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 18:59:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 18:59:55]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Marijn Kruisselbrink, 2017-04-29 00:34:12]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 00:34:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 01:16:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-29 01:16:22]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Marijn Kruisselbrink, 2017-05-01 18:53:05]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 18:53:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 19:47:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 19:47:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marijn Kruisselbrink, 2017-05-01 21:06:01]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 21:06:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 22:19:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 22:19:09]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Marijn Kruisselbrink, 2017-05-05 19:41:56]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 19:42:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 19:54:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 19:54:22]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marijn Kruisselbrink, 2017-05-05 20:19:29]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 20:20:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 21:22:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 21:22:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marijn Kruisselbrink, 2017-05-09 00:34:23]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 00:40:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 02:46:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 02:46:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marijn Kruisselbrink, 2017-05-15 18:33:56]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 18:35:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 19:26:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 19:26:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Marijn Kruisselbrink, 2017-05-15 21:29:25]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 21:30:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 23:04:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 23:04:38]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marijn Kruisselbrink, 2017-05-19 16:43:33]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 16:44:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 19:17:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 19:17:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marijn Kruisselbrink, 2017-05-25 17:36:16]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 17:36:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 19:52:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 19:52:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marijn Kruisselbrink, 2017-05-25 20:05:34]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 20:06:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2017-05-25 23:47:36]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 23:48:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2017-05-25 23:56:38]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 23:58:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2017-05-26 00:02:54]

Description was changed from

==========
Switch to mojo localstorage backend by default.

Old non-mojo sqlite based backend is still available with
--disable-mojo-local-storage. The mojo-localstorage virtual layout test
suite is switched to now test the non-mojo local storage implementation.

BUG=586194
==========

to

==========
Switch to mojo localstorage backend by default.

Old non-mojo sqlite based backend is still available with
--disable-mojo-local-storage. The mojo-localstorage virtual layout test
suite is switched to now test the non-mojo local storage implementation.

Other than changing the default, some minor changes are made to make
sure all existing tests still pass:
- LocalStorageContextMojo is made to work in unit tests where no Connector
  exists
- DOMStorageContextWrapper clears out its DOMStorageContextImpl pointer
  on shutdown to fix some issues around shutdown when a request for local
  storage usage is still being processed while shutdown is in process.
- Several StoragePartitionImpl unittests relied on localstorage internals,
  these tests were updated to now work with the new leveldb implementation.

BUG=586194
==========

[Marijn Kruisselbrink, 2017-05-26 00:03:19]

Patchset #16 (id:300001) has been deleted

[Marijn Kruisselbrink, 2017-05-26 00:03:59]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 00:05:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2017-05-26 00:05:49]

mek@chromium.org changed reviewers:
+ michaeln@chromium.org

[commit-bot: I haz the power, 2017-05-26 02:44:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 02:45:00]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:274: context_ =
nullptr;
this might turn up some crashes in odd cases where something unexpected calls us
after shutdown?

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:352: if (!context_)
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?

https://codereview.chromium.org/2847013002/diff/320001/content/browser/storag...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/2847013002/diff/320001/content/browser/storag...
content/browser/storage_partition_impl_unittest.cc:204:
->LocalStorageDatabaseRequestForTesting());
This is such a bizarre way to inject a mock instance of something? mojo is so
wierd? I'd expect to pass things in rather than ask for something out?

context->SetDatabaseForTesting(&mock_db_.binding_);

[Marijn Kruisselbrink, 2017-05-26 22:05:53]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 22:06:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 22:10:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 22:10:08]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marijn Kruisselbrink, 2017-05-26 22:31:01]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 22:31:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:274: context_ =
nullptr;
On 2017/05/26 at 19:12:44, michaeln wrote:
> this might turn up some crashes in odd cases where something unexpected calls
us after shutdown?

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...

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:352: if (!context_)
On 2017/05/26 at 19:12:44, michaeln wrote:
> we might need more early outs like this to protect agains UAF
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).


> 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?


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).

https://codereview.chromium.org/2847013002/diff/320001/content/browser/storag...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/2847013002/diff/320001/content/browser/storag...
content/browser/storage_partition_impl_unittest.cc:204:
->LocalStorageDatabaseRequestForTesting());
On 2017/05/26 at 19:12:44, michaeln wrote:
> This is such a bizarre way to inject a mock instance of something? mojo is so
wierd? I'd expect to pass things in rather than ask for something out?
> 
> context->SetDatabaseForTesting(&mock_db_.binding_);

Generally there is two directions in which you can setup a connection between an
InterfacePtr and a Binding in mojo. You can either create the InterfacePtr
first, call MakeRequest on it and pass that Request to the Binding, or you can
create the Binding, and call CreateInterfacePtrAndBind on it to get a
InterfacePtr. In non-test code it's usually pretty obvious which one you want,
since either code is requesing an interface from something else, or code is
giving an interface to somewhere that hasn't asked for it yet.

Not sure why I originally wrote the testing code in this weird "ask for a
request for a binding" pattern though... Probably because AssociatedBinding
doesn't have the equivalent to CreateInterfacePtrAndBind so you have to write a
few more lines of code to do the equivalent. But actually writing those few
extra lines isn't that complicated, so changed it all to the more sensible
"provide an interfaceptr" pattern.

[commit-bot: I haz the power, 2017-05-27 00:45:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 00:45:22]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-05-27 01:59:04]

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:274: context_ =
nullptr;
On 2017/05/26 23:43:02, Marijn Kruisselbrink wrote:
> On 2017/05/26 at 19:12:44, michaeln wrote:
> > this might turn up some crashes in odd cases where something unexpected
calls
> us after shutdown?
> 
> 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...

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

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:352: if (!context_)
On 2017/05/26 23:43:02, Marijn Kruisselbrink wrote:
> On 2017/05/26 at 19:12:44, michaeln wrote:
> > we might need more early outs like this to protect agains UAF
> 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).

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

> > 
> > 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?
> 
> 
> 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).

that's a lot nicer!

https://codereview.chromium.org/2847013002/diff/320001/content/browser/storag...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/2847013002/diff/320001/content/browser/storag...
content/browser/storage_partition_impl_unittest.cc:204:
->LocalStorageDatabaseRequestForTesting());
On 2017/05/26 23:43:02, Marijn Kruisselbrink wrote:
> On 2017/05/26 at 19:12:44, michaeln wrote:
> > This is such a bizarre way to inject a mock instance of something? mojo is
so
> wierd? I'd expect to pass things in rather than ask for something out?
> > 
> > context->SetDatabaseForTesting(&mock_db_.binding_);
> 
> Generally there is two directions in which you can setup a connection between
an
> InterfacePtr and a Binding in mojo. You can either create the InterfacePtr
> first, call MakeRequest on it and pass that Request to the Binding, or you can
> create the Binding, and call CreateInterfacePtrAndBind on it to get a
> InterfacePtr. In non-test code it's usually pretty obvious which one you want,
> since either code is requesing an interface from something else, or code is
> giving an interface to somewhere that hasn't asked for it yet.
> 
> Not sure why I originally wrote the testing code in this weird "ask for a
> request for a binding" pattern though... Probably because AssociatedBinding
> doesn't have the equivalent to CreateInterfacePtrAndBind so you have to write
a
> few more lines of code to do the equivalent. But actually writing those few
> extra lines isn't that complicated, so changed it all to the more sensible
> "provide an interfaceptr" pattern.

thnx

[Marijn Kruisselbrink, 2017-05-30 20:53:56]

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/2847013002/diff/320001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:352: if (!context_)
On 2017/05/27 at 01:59:03, michaeln wrote:
> On 2017/05/26 23:43:02, Marijn Kruisselbrink wrote:
> > On 2017/05/26 at 19:12:44, michaeln wrote:
> > > we might need more early outs like this to protect agains UAF
> > 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).
> 
> 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

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_...

[Marijn Kruisselbrink, 2017-05-30 22:21:42]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 22:22:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 00:18:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 00:19:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marijn Kruisselbrink, 2017-06-01 18:09:46]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 18:10:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 20:37:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 20:37:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marijn Kruisselbrink, 2017-06-02 21:49:20]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 21:49:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2017-06-02 22:21:48]

Description was changed from

==========
Switch to mojo localstorage backend by default.

Old non-mojo sqlite based backend is still available with
--disable-mojo-local-storage. The mojo-localstorage virtual layout test
suite is switched to now test the non-mojo local storage implementation.

Other than changing the default, some minor changes are made to make
sure all existing tests still pass:
- LocalStorageContextMojo is made to work in unit tests where no Connector
  exists
- DOMStorageContextWrapper clears out its DOMStorageContextImpl pointer
  on shutdown to fix some issues around shutdown when a request for local
  storage usage is still being processed while shutdown is in process.
- Several StoragePartitionImpl unittests relied on localstorage internals,
  these tests were updated to now work with the new leveldb implementation.

BUG=586194
==========

to

==========
Switch to mojo localstorage backend by default.

Old non-mojo sqlite based backend is still available with
--disable-mojo-local-storage. The mojo-localstorage virtual layout test
suite is switched to now test the non-mojo local storage implementation.

Other than changing the default, some minor changes are made to make
sure all existing tests still pass:
- LocalStorageContextMojo is made to work in unit tests where no Connector
  exists
- Several StoragePartitionImpl unittests relied on localstorage internals,
  these tests were updated to now work with the new leveldb implementation.

BUG=586194
==========

[Marijn Kruisselbrink, 2017-06-02 23:03:34]

okay, the TaskScheduler DCHECK I was hitting in the one unit tests was removed,
so there is no longer the need to null out context_ or anything similar. I think
this should be good to go like this...

[commit-bot: I haz the power, 2017-06-03 00:08:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-03 00:08:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Marijn Kruisselbrink, 2017-06-05 19:32:52]

On 2017/06/02 at 23:03:34, Marijn Kruisselbrink wrote:
> okay, the TaskScheduler DCHECK I was hitting in the one unit tests was
removed, so there is no longer the need to null out context_ or anything
similar. I think this should be good to go like this...

michaeln: does this look good to you now?

[michaeln, 2017-06-05 20:18:07]

> michaeln: does this look good to you now?

lgtm

[Marijn Kruisselbrink, 2017-06-05 20:24:18]

mek@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org

[Marijn Kruisselbrink, 2017-06-05 20:24:19]

+jam for content OWNERS
+dcheng for security review as requested in
content/browser/renderer_host/render_process_host_impl.cc

[jam, 2017-06-06 17:15:58]

On 2017/06/05 20:24:19, Marijn Kruisselbrink wrote:
> +jam for content OWNERS
> +dcheng for security review as requested in
> content/browser/renderer_host/render_process_host_impl.cc

lgtm, awesome!

[Marijn Kruisselbrink, 2017-06-06 18:23:36]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 18:24:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 20:32:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 20:32:09]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2017-06-06 20:47:53]

dcheng: ping?

[dcheng, 2017-06-06 22:22:56]

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:52: auto infos =
base::MakeUnique<std::vector<LocalStorageUsageInfo>>();
Out of curiosity: why wrap it in unique_ptr? vector is movable (and we appear to
take advantage of this just a few lines below)

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
content/browser/dom_storage/local_storage_context_mojo.cc:391: // happens.
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.)

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
content/browser/dom_storage/local_storage_context_mojo.cc:617: // used, so
return an empty array.
Does this mean the renderer did something bad?

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

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:52: auto infos =
base::MakeUnique<std::vector<LocalStorageUsageInfo>>();
On 2017/06/06 at 22:22:55, dcheng wrote:
> Out of curiosity: why wrap it in unique_ptr? vector is movable (and we appear
to take advantage of this just a few lines below)

Good question... The old code presumably wrapped it in a base::Owned raw pointer
because when it was written vector wasn't moveable and unique_ptr didn't exist.
The reason I kept it wrapped, but now in a unique_ptr is so
InvokeLocalStorageUsageCallbackHelper can be shared between both the sqlite-only
and the sqlite&leveldb/mojo code-path.

In that case in the GetLocalStorage implementation I want to be able to pass a
(raw pointer) to the same vector to two different callbacks, after which
InvokeLocalStorageUsageCallbackHelper gets called with the unique_ptr for that
same vector. If instead I just moved the vector itself to the callback that
binds InvokeLocalStorageUsageCallbackHelper I wouldn't be able to pass the
address to the vector also to the two separate callbacks (for getting sqlite
usage and getting leveldb usage).

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
content/browser/dom_storage/local_storage_context_mojo.cc:391: // happens.
On 2017/06/06 at 22:22:55, dcheng wrote:
> 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?
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...

> (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.)

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
content/browser/dom_storage/local_storage_context_mojo.cc:617: // used, so
return an empty array.
On 2017/06/06 at 22:22:55, dcheng wrote:
> Does this mean the renderer did something bad?

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).

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

ipc lgtm, thanks

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:52: auto infos =
base::MakeUnique<std::vector<LocalStorageUsageInfo>>();
On 2017/06/06 22:52:35, Marijn Kruisselbrink wrote:
> On 2017/06/06 at 22:22:55, dcheng wrote:
> > Out of curiosity: why wrap it in unique_ptr? vector is movable (and we
appear
> to take advantage of this just a few lines below)
> 
> Good question... The old code presumably wrapped it in a base::Owned raw
pointer
> because when it was written vector wasn't moveable and unique_ptr didn't
exist.
> The reason I kept it wrapped, but now in a unique_ptr is so
> InvokeLocalStorageUsageCallbackHelper can be shared between both the
sqlite-only
> and the sqlite&leveldb/mojo code-path.
> 
> In that case in the GetLocalStorage implementation I want to be able to pass a
> (raw pointer) to the same vector to two different callbacks, after which
> InvokeLocalStorageUsageCallbackHelper gets called with the unique_ptr for that
> same vector. If instead I just moved the vector itself to the callback that
> binds InvokeLocalStorageUsageCallbackHelper I wouldn't be able to pass the
> address to the vector also to the two separate callbacks (for getting sqlite
> usage and getting leveldb usage).

Ah, I see. The code felt kind of like it wanted promises. One thing I'm not sure
about is why we need to call both GetLocalStorageUsageHelper and
GetMojoLocalStorageUsage (i.e. what's the difference), so perhaps some comments
would help with making this part of the code more understandable to readers who
aren't familiar with it.

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2847013002/diff/400001/content/browser/dom_st...
content/browser/dom_storage/local_storage_context_mojo.cc:391: // happens.
On 2017/06/06 22:52:35, Marijn Kruisselbrink wrote:
> On 2017/06/06 at 22:22:55, dcheng wrote:
> > 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?
> 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...
> 
> > (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.)
> 

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.

[Marijn Kruisselbrink, 2017-06-06 23:13:23]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2017-06-06 23:13:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 23:18:31]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1496790803690990,
"parent_rev": "9bb3bafb3347ac26c6dbbf78f9b77198f71eb8d5", "commit_rev":
"c8e2b27f71c6792fa1072fa98a22c111ad229090"}

[commit-bot: I haz the power, 2017-06-06 23:18:42]

Description was changed from

==========
Switch to mojo localstorage backend by default.

Old non-mojo sqlite based backend is still available with
--disable-mojo-local-storage. The mojo-localstorage virtual layout test
suite is switched to now test the non-mojo local storage implementation.

Other than changing the default, some minor changes are made to make
sure all existing tests still pass:
- LocalStorageContextMojo is made to work in unit tests where no Connector
  exists
- Several StoragePartitionImpl unittests relied on localstorage internals,
  these tests were updated to now work with the new leveldb implementation.

BUG=586194
==========

to

==========
Switch to mojo localstorage backend by default.

Old non-mojo sqlite based backend is still available with
--disable-mojo-local-storage. The mojo-localstorage virtual layout test
suite is switched to now test the non-mojo local storage implementation.

Other than changing the default, some minor changes are made to make
sure all existing tests still pass:
- LocalStorageContextMojo is made to work in unit tests where no Connector
  exists
- Several StoragePartitionImpl unittests relied on localstorage internals,
  these tests were updated to now work with the new leveldb implementation.

BUG=586194

Review-Url: https://codereview.chromium.org/2847013002
Cr-Commit-Position: refs/heads/master@{#477458}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8e2b27f71c6792fa1072fa98a22...
==========

[commit-bot: I haz the power, 2017-06-06 23:18:44]

Committed patchset #20 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/c8e2b27f71c6792fa1072fa98a22...