Enable mojo localstorage to actually store on disk.

Enable mojo localstorage to actually store on disk.

Includes a browser test to make sure this works, as well as some fixes
needed to make it work:

LevelDBMojoProxy makes sync mojo calls, but runs in the browser process,
so add ScopedAllowSyncCall to all the places it makes sync calls.

If the browser quits before the connection to the LevelDB database is
completely setup, it isn't possible anymore to get the connection working.
So we need to make sure that the connection is always ready before a renderer
tries to make changes to the stored data. For most methods this was already the
case as we always (sync) load all existing data for an origin, but if the first
change a renderer makes is clear(), there isn't a need to actually load all the
data. To make sure the connection is setup anyway, this CL adds a new sync
method to LevelDBWrapper purely to block the renderer until the database is
ready.

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

[Marijn Kruisselbrink, 2017-01-23 18:46:33]

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

[commit-bot: I haz the power, 2017-01-23 18:47:17]

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-01-23 20:35:53]

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

[commit-bot: I haz the power, 2017-01-23 20:35:54]

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

[Marijn Kruisselbrink, 2017-01-23 23:18:20]

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

[commit-bot: I haz the power, 2017-01-23 23:19: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-01-24 00:18:37]

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

[commit-bot: I haz the power, 2017-01-24 00:19:23]

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-01-24 00:47:15]

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

[commit-bot: I haz the power, 2017-01-24 00:47:56]

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-01-24 01:58:36]

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

[commit-bot: I haz the power, 2017-01-24 01:58:38]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2017-01-24 19:33:15]

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

[Marijn Kruisselbrink, 2017-01-24 19:33:16]

I believe the ScopedAllowSyncCall bits in LevelDBMojoProxy should be safe (and
there isn't really a way for the leveldb service to work without them), but it's
kind of annoying that they are needed... 

Also not entirely happy with the hack that is the LevelDBWrapper.Sync method. It
is needed to make sure that we at least have a connection to the leveldb
database before javascript tries to make changes (otherwise if the browser shuts
down shortly after making changes there is no way to persist those changes, as
actually trying to (finish) setting up a connection during browser shutdown is
not going to work. I could of course just call GetAll from
LocalStorageCachedArea::Clear(), like all the other methods that modify the
database, but that seems even more wasteful.

[jam, 2017-01-25 00:42:30]

Description was changed from

==========
Enable mojo localstorage to actually store on disk.

Includes a browser test to make sure this works, as well as some fixes
needed to make it work:

LevelDBMojoProxy makes sync mojo calls, but runs in the browser process,
so add ScopedAllowSyncCall to all the places it makes sync calls.

If the browser quits before the connection to the LevelDB database is
completely setup, it isn't possible anymore to get the connection working.
So we need to make sure that the connection is always ready before a renderer
tries to make changes to the stored data. For most methods this was already the
case as we always (sync) load all existing data for an origin, but if the first
change a renderer makes is clear(), there isn't a need to actually load all the
data. To make sure the connection is setup anyway, this CL adds a new sync
method to LevelDBWrapper purely to block the renderer until the database is
ready.

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

to

==========
Enable mojo localstorage to actually store on disk.

Includes a browser test to make sure this works, as well as some fixes
needed to make it work:

LevelDBMojoProxy makes sync mojo calls, but runs in the browser process,
so add ScopedAllowSyncCall to all the places it makes sync calls.

If the browser quits before the connection to the LevelDB database is
completely setup, it isn't possible anymore to get the connection working.
So we need to make sure that the connection is always ready before a renderer
tries to make changes to the stored data. For most methods this was already the
case as we always (sync) load all existing data for an origin, but if the first
change a renderer makes is clear(), there isn't a need to actually load all the
data. To make sure the connection is setup anyway, this CL adds a new sync
method to LevelDBWrapper purely to block the renderer until the database is
ready.

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

[jam, 2017-01-25 00:42:30]

jam@chromium.org changed reviewers:
+ yzshen@chromium.org

[jam, 2017-01-25 00:46:55]

lgtm

On 2017/01/24 19:33:16, Marijn Kruisselbrink wrote:
> I believe the ScopedAllowSyncCall bits in LevelDBMojoProxy should be safe (and
> there isn't really a way for the leveldb service to work without them), but
it's
> kind of annoying that they are needed... 

Yuzhu: is there a way to avoid adding ScopedAllowSyncCall when we know that the
other endpoint is in the same process? I realize MPs could move around, but
since this is a debugging aid, seems fine if we could just check at sending time
whether the other endpoint is in process or not.

> 
> Also not entirely happy with the hack that is the LevelDBWrapper.Sync method.
It
> is needed to make sure that we at least have a connection to the leveldb
> database before javascript tries to make changes (otherwise if the browser
shuts
> down shortly after making changes there is no way to persist those changes, as
> actually trying to (finish) setting up a connection during browser shutdown is
> not going to work. I could of course just call GetAll from
> LocalStorageCachedArea::Clear(), like all the other methods that modify the
> database, but that seems even more wasteful.

Do we care about persisting data in this case at all since this is racy by
definition?

https://codereview.chromium.org/2649963002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/sync_call_restrictions.h (right):

https://codereview.chromium.org/2649963002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/sync_call_restrictions.h:59: friend class
leveldb::LevelDBMojoProxy;
nit: add comment saying this is in-process

[michaeln, 2017-01-25 02:28:38]

https://codereview.chromium.org/2649963002/diff/60001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2649963002/diff/60001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:158: leveldb_->Sync();
that's unfortunate?

would an async call to storage_partition_service->ClearLocalStorage that does
not rely on the level_db_ instance being setup help avoid this Sync() call?

[yzshen1, 2017-01-25 17:41:59]

On 2017/01/25 00:46:55, jam wrote:
> lgtm
> 
> On 2017/01/24 19:33:16, Marijn Kruisselbrink wrote:
> > I believe the ScopedAllowSyncCall bits in LevelDBMojoProxy should be safe
(and
> > there isn't really a way for the leveldb service to work without them), but
> it's
> > kind of annoying that they are needed... 
> 
> Yuzhu: is there a way to avoid adding ScopedAllowSyncCall when we know that
the
> other endpoint is in the same process? I realize MPs could move around, but
> since this is a debugging aid, seems fine if we could just check at sending
time
> whether the other endpoint is in process or not.

There is currently no such support.

Even if the two endpoints are in the same process, we may not always want to
allow sync call. For example, the UI thread tries to make a sync call whose
implementation lives on the IO thread. WDYT? Thanks!

> 
> > 
> > Also not entirely happy with the hack that is the LevelDBWrapper.Sync
method.
> It
> > is needed to make sure that we at least have a connection to the leveldb
> > database before javascript tries to make changes (otherwise if the browser
> shuts
> > down shortly after making changes there is no way to persist those changes,
as
> > actually trying to (finish) setting up a connection during browser shutdown
is
> > not going to work. I could of course just call GetAll from
> > LocalStorageCachedArea::Clear(), like all the other methods that modify the
> > database, but that seems even more wasteful.
> 
> Do we care about persisting data in this case at all since this is racy by
> definition?
> 
>
https://codereview.chromium.org/2649963002/diff/60001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/sync_call_restrictions.h (right):
> 
>
https://codereview.chromium.org/2649963002/diff/60001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/sync_call_restrictions.h:59: friend class
> leveldb::LevelDBMojoProxy;
> nit: add comment saying this is in-process

[jam, 2017-01-25 18:38:33]

On 2017/01/25 17:41:59, yzshen1 wrote:
> On 2017/01/25 00:46:55, jam wrote:
> > lgtm
> > 
> > On 2017/01/24 19:33:16, Marijn Kruisselbrink wrote:
> > > I believe the ScopedAllowSyncCall bits in LevelDBMojoProxy should be safe
> (and
> > > there isn't really a way for the leveldb service to work without them),
but
> > it's
> > > kind of annoying that they are needed... 
> > 
> > Yuzhu: is there a way to avoid adding ScopedAllowSyncCall when we know that
> the
> > other endpoint is in the same process? I realize MPs could move around, but
> > since this is a debugging aid, seems fine if we could just check at sending
> time
> > whether the other endpoint is in process or not.
> 
> There is currently no such support.
> 
> Even if the two endpoints are in the same process, we may not always want to
> allow sync call. For example, the UI thread tries to make a sync call whose
> implementation lives on the IO thread. WDYT? Thanks!

Good point. ignore my question then :)

[Marijn Kruisselbrink, 2017-01-25 22:00:05]

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

[Marijn Kruisselbrink, 2017-01-25 22:00:08]

On 2017/01/25 at 18:38:33, jam wrote:
> On 2017/01/25 17:41:59, yzshen1 wrote:
> > Even if the two endpoints are in the same process, we may not always want to
> > allow sync call. For example, the UI thread tries to make a sync call whose
> > implementation lives on the IO thread. WDYT? Thanks!
> 
> Good point. ignore my question then :)

Presumably in that case you'd still check base::ThreadRestrictions, and only
allow sync calls from threads where waiting is allowed?

https://codereview.chromium.org/2649963002/diff/60001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2649963002/diff/60001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:158: leveldb_->Sync();
On 2017/01/25 at 02:28:38, michaeln wrote:
> that's unfortunate?
Yes, it is. I don't really see a way around it though unfortunately. At least it
seems from my limited testing that committing changes still works fine even if
that has to happen during browser shutdown. But setting up the connection to
leveldb, verifying its schema version etc just doesn't, probably partially
because of the many thread hops involved.

> would an async call to storage_partition_service->ClearLocalStorage that does
not rely on the level_db_ instance being setup help avoid this Sync() call?

The whole reason for the sync call is to make sure the level db instance is
setup. I would kind of expect most real life usage of localstorage to not start
out by clearing it all, so the performance impact of this should be fairly
minimal. I just want to avoid possibly losing all writes in a situation where a
page happens to start out by clearing storage. Specifically, without this sync
call here the browser test I've added doesn't pass because the browser shuts
down somewhere in the middle of still setting up the connection to the database.

Also not sure if it is guaranteed that committing things during shutdown works;
it just seems StoragePartitions are destroyed early enough in the shutdown
process for things to still work. But I don't quite understand what all the
threads are that are involved, and in what order, so it's very well possible
that there still are situations where changes that javascript thinks were
written end up getting lost anyway. More research is necessary, but at least
starting out from a situation where uncommitted changes can only exist if there
is a connection to the database makes it more plausible that this actually is a
solvable problem.

https://codereview.chromium.org/2649963002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/sync_call_restrictions.h (right):

https://codereview.chromium.org/2649963002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/sync_call_restrictions.h:59: friend class
leveldb::LevelDBMojoProxy;
On 2017/01/25 at 00:46:55, jam wrote:
> nit: add comment saying this is in-process

Done

[commit-bot: I haz the power, 2017-01-25 22:00:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaeln, 2017-01-25 23:53:28]

lgtm

https://codereview.chromium.org/2649963002/diff/60001/content/renderer/dom_st...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2649963002/diff/60001/content/renderer/dom_st...
content/renderer/dom_storage/local_storage_cached_area.cc:158: leveldb_->Sync();
On 2017/01/25 22:00:07, Marijn Kruisselbrink wrote:
> On 2017/01/25 at 02:28:38, michaeln wrote:
> > that's unfortunate?
> Yes, it is. I don't really see a way around it though unfortunately. At least
it
> seems from my limited testing that committing changes still works fine even if
> that has to happen during browser shutdown. But setting up the connection to
> leveldb, verifying its schema version etc just doesn't, probably partially
> because of the many thread hops involved.
> 
> > would an async call to storage_partition_service->ClearLocalStorage that
does
> not rely on the level_db_ instance being setup help avoid this Sync() call?
> 
> The whole reason for the sync call is to make sure the level db instance is
> setup. I would kind of expect most real life usage of localstorage to not
start
> out by clearing it all, so the performance impact of this should be fairly
> minimal. I just want to avoid possibly losing all writes in a situation where
a
> page happens to start out by clearing storage. Specifically, without this sync
> call here the browser test I've added doesn't pass because the browser shuts
> down somewhere in the middle of still setting up the connection to the
database.
> 
> Also not sure if it is guaranteed that committing things during shutdown
works;
> it just seems StoragePartitions are destroyed early enough in the shutdown
> process for things to still work. But I don't quite understand what all the
> threads are that are involved, and in what order, so it's very well possible
> that there still are situations where changes that javascript thinks were
> written end up getting lost anyway. More research is necessary, but at least
> starting out from a situation where uncommitted changes can only exist if
there
> is a connection to the database makes it more plausible that this actually is
a
> solvable problem.

I see, the problem is not the renderer-to-browser connection to the
leveldb_wrapper, but the browser processes internal connection from
mojocontext-to-LevelDBDatabase.

About the general problem of committed during shutdown, the way it works on CrOS
is problematic. There's a very limited about of time before the process is
killed and its completely racy whether localstorage data is flushed or not. A
long time ago I started looking at a change to BrowserProcessImpl::EndSession to
at least try to wait for localstorage to be flushed.

https://codereview.chromium.org/1140013003/diff/80001/chrome/browser/browser_...

[commit-bot: I haz the power, 2017-01-26 01:29:03]

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

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

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

[yzshen1, 2017-01-26 18:07:13]

On 2017/01/25 22:00:08, Marijn Kruisselbrink wrote:
> On 2017/01/25 at 18:38:33, jam wrote:
> > On 2017/01/25 17:41:59, yzshen1 wrote:
> > > Even if the two endpoints are in the same process, we may not always want
to
> > > allow sync call. For example, the UI thread tries to make a sync call
whose
> > > implementation lives on the IO thread. WDYT? Thanks!
> > 
> > Good point. ignore my question then :)
> 
> Presumably in that case you'd still check base::ThreadRestrictions, and only
> allow sync calls from threads where waiting is allowed?

Correct. base::ThreadRestrictions is respected because sync call relies on
condition variable from base.
However, it would seems a little inconsistent and may cause more confusion:
- If the sync call is going to another process, ScopedAllowSyncCall is needed.
- If the sync call is in-process but wait is not allowed by default on this
thread, ScopedAllowWait is needed.
- If the sync call is in-process but wait is allowed by default on this thread,
neither is needed.


>
https://codereview.chromium.org/2649963002/diff/60001/content/renderer/dom_st...
> File content/renderer/dom_storage/local_storage_cached_area.cc (right):
> 
>
https://codereview.chromium.org/2649963002/diff/60001/content/renderer/dom_st...
> content/renderer/dom_storage/local_storage_cached_area.cc:158:
leveldb_->Sync();
> On 2017/01/25 at 02:28:38, michaeln wrote:
> > that's unfortunate?
> Yes, it is. I don't really see a way around it though unfortunately. At least
it
> seems from my limited testing that committing changes still works fine even if
> that has to happen during browser shutdown. But setting up the connection to
> leveldb, verifying its schema version etc just doesn't, probably partially
> because of the many thread hops involved.
> 
> > would an async call to storage_partition_service->ClearLocalStorage that
does
> not rely on the level_db_ instance being setup help avoid this Sync() call?
> 
> The whole reason for the sync call is to make sure the level db instance is
> setup. I would kind of expect most real life usage of localstorage to not
start
> out by clearing it all, so the performance impact of this should be fairly
> minimal. I just want to avoid possibly losing all writes in a situation where
a
> page happens to start out by clearing storage. Specifically, without this sync
> call here the browser test I've added doesn't pass because the browser shuts
> down somewhere in the middle of still setting up the connection to the
database.
> 
> Also not sure if it is guaranteed that committing things during shutdown
works;
> it just seems StoragePartitions are destroyed early enough in the shutdown
> process for things to still work. But I don't quite understand what all the
> threads are that are involved, and in what order, so it's very well possible
> that there still are situations where changes that javascript thinks were
> written end up getting lost anyway. More research is necessary, but at least
> starting out from a situation where uncommitted changes can only exist if
there
> is a connection to the database makes it more plausible that this actually is
a
> solvable problem.
> 
>
https://codereview.chromium.org/2649963002/diff/60001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/sync_call_restrictions.h (right):
> 
>
https://codereview.chromium.org/2649963002/diff/60001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/sync_call_restrictions.h:59: friend class
> leveldb::LevelDBMojoProxy;
> On 2017/01/25 at 00:46:55, jam wrote:
> > nit: add comment saying this is in-process
> 
> Done

[Marijn Kruisselbrink, 2017-01-30 19:01:00]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2017-01-30 19:01:01]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2649963002/#ps80001
(title: "add sync call restrictions comment")

[commit-bot: I haz the power, 2017-01-30 19:01:28]

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

[Marijn Kruisselbrink, 2017-01-30 19:08:56]

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

[Marijn Kruisselbrink, 2017-01-30 19:08:57]

+dcheng for content/common/leveldb_wrapper.mojom OWNERS
+isherman for histograms.xml OWNERS

[commit-bot: I haz the power, 2017-01-30 19:12:31]

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

[commit-bot: I haz the power, 2017-01-30 19:12:32]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ilya Sherman, 2017-01-30 19:40:28]

Metrics LGTM

[dcheng, 2017-01-31 05:42:58]

This might be a dumb question, but why don't we just return the wrapper through
a factory interface? Wouldn't that guarantee that it's always initialized then?

[Marijn Kruisselbrink, 2017-01-31 23:17:39]

On 2017/01/31 at 05:42:58, dcheng wrote:
> This might be a dumb question, but why don't we just return the wrapper
through a factory interface? Wouldn't that guarantee that it's always
initialized then?

I'm not sure how that would help anything? The LevelDBWrapper is returned by the
StoragePartitionService OpenLocalStorage method. That method could be changed to
return the LevelDBWrapper instance rather than a request being passed into it,
but that wouldn't fundamentally change anything. Renderer side code would then
still have to wait/block until it receives the instance before it can do
anything. Also fwiw, in nearly all cases a renderer will send a (sync) GetAll
message to the LevelDBWrapper as the first thing it does. It is only in the case
where the first thing a renderer does to localstorage for a particular origin is
a "clear" that the GetAll thing is skipped, and in this CL replaced with the
"dummy" Sync call. (for what it's worth, the old IPC localstorage implementation
also has a dummy sync IPC, although that one is used at other points, to block
the renderer if it gets too far behind the browser/IPC layer. I haven't quite
decided yet if we'll need something similar here. Doe to differences in how sync
and async messages are interleaved in IPC vs mojo it's not as straight forward
to use a sync message to block until all previous async messages have been
processed. But that's effectively what I'm trying to do).

[Marijn Kruisselbrink, 2017-02-07 21:42:49]

dcheng: ping?

[dcheng, 2017-02-07 23:39:10]

I don't want to block this change any longer, but I feel like we're not using
Mojo quite right here: if we need this interface to be completely bound, then
the right thing to do (IMO) is return an InterfacePtr, not pass an
InterfaceRequest.

Future changes can easily introduce new calls which don't happen to have a sync
call in the path, and won't force this sync setup. Whereas if we change
OpenLocalStorage() to be [Sync] and return an InterfacePtr, we'll solve this
problem centrally. I don't see a lot of value in returning the interface async
if you need to immediately make a sync call to guarantee validity.

[dcheng, 2017-02-07 23:40:15]

On 2017/02/07 23:39:10, dcheng wrote:
> I don't want to block this change any longer, but I feel like we're not using
> Mojo quite right here: if we need this interface to be completely bound, then
> the right thing to do (IMO) is return an InterfacePtr, not pass an
> InterfaceRequest.
> 
> Future changes can easily introduce new calls which don't happen to have a
sync
> call in the path, and won't force this sync setup. Whereas if we change
> OpenLocalStorage() to be [Sync] and return an InterfacePtr, we'll solve this
> problem centrally. I don't see a lot of value in returning the interface async
> if you need to immediately make a sync call to guarantee validity.

And if for some reason, we can't do this, let's document this in the CL
description / mojom interface.

[Marijn Kruisselbrink, 2017-02-08 00:29:28]

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

[commit-bot: I haz the power, 2017-02-08 00:30:35]

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-02-08 00:33:12]

On 2017/02/07 at 23:40:15, dcheng wrote:
> On 2017/02/07 23:39:10, dcheng wrote:
> > I don't want to block this change any longer, but I feel like we're not
using
> > Mojo quite right here: if we need this interface to be completely bound,
then
> > the right thing to do (IMO) is return an InterfacePtr, not pass an
> > InterfaceRequest.
> > 
> > Future changes can easily introduce new calls which don't happen to have a
sync
> > call in the path, and won't force this sync setup. Whereas if we change
> > OpenLocalStorage() to be [Sync] and return an InterfacePtr, we'll solve this
> > problem centrally. I don't see a lot of value in returning the interface
async
> > if you need to immediately make a sync call to guarantee validity.
> 
> And if for some reason, we can't do this, let's document this in the CL
description / mojom interface.

I could have done that, but there would still be the second GetAll sync call in
most cases after that, so it would on average be a performance hit. But anyway,
I became convinced that this Sync thing indeed wasn't the way to go. The only
case it really addresses is if the first page that uses localstorage in a
particular session happens to call clear() as its first localstorage access, and
then the browser closes almost right away. That's such a rare case that I've
began to agree that just making sure the browsertest flushes data is better.
There are still other issues around committing data during browser shutdown, but
those weren't helped by this Sync thing anyway.

So in this CL I removed the mojom changes, and instead added some extra logic to
the test.

[commit-bot: I haz the power, 2017-02-08 02:55:45]

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

[commit-bot: I haz the power, 2017-02-08 02:55:48]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2017-02-08 17:19:50]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2017-02-08 17:19:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, isherman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2649963002/#ps100001
(title: "Remove Sync() method, and instead make sure browser test waits long
enough.")

[commit-bot: I haz the power, 2017-02-08 17:20:20]

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-02-08 17:25:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486574390927970,
"parent_rev": "a163274510eac87e8d5aef4e4d695af3f7e10064", "commit_rev":
"3ea335139f6f337d8cc46250fd86f8965f6d0a53"}

[commit-bot: I haz the power, 2017-02-08 17:26:20]

Description was changed from

==========
Enable mojo localstorage to actually store on disk.

Includes a browser test to make sure this works, as well as some fixes
needed to make it work:

LevelDBMojoProxy makes sync mojo calls, but runs in the browser process,
so add ScopedAllowSyncCall to all the places it makes sync calls.

If the browser quits before the connection to the LevelDB database is
completely setup, it isn't possible anymore to get the connection working.
So we need to make sure that the connection is always ready before a renderer
tries to make changes to the stored data. For most methods this was already the
case as we always (sync) load all existing data for an origin, but if the first
change a renderer makes is clear(), there isn't a need to actually load all the
data. To make sure the connection is setup anyway, this CL adds a new sync
method to LevelDBWrapper purely to block the renderer until the database is
ready.

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

to

==========
Enable mojo localstorage to actually store on disk.

Includes a browser test to make sure this works, as well as some fixes
needed to make it work:

LevelDBMojoProxy makes sync mojo calls, but runs in the browser process,
so add ScopedAllowSyncCall to all the places it makes sync calls.

If the browser quits before the connection to the LevelDB database is
completely setup, it isn't possible anymore to get the connection working.
So we need to make sure that the connection is always ready before a renderer
tries to make changes to the stored data. For most methods this was already the
case as we always (sync) load all existing data for an origin, but if the first
change a renderer makes is clear(), there isn't a need to actually load all the
data. To make sure the connection is setup anyway, this CL adds a new sync
method to LevelDBWrapper purely to block the renderer until the database is
ready.

BUG=586194

Review-Url: https://codereview.chromium.org/2649963002
Cr-Commit-Position: refs/heads/master@{#449021}
Committed:
https://chromium.googlesource.com/chromium/src/+/3ea335139f6f337d8cc46250fd86...
==========

[commit-bot: I haz the power, 2017-02-08 17:26:22]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3ea335139f6f337d8cc46250fd86...