Delete and try to recreate localstorage database on invalid schema version.

Delete and try to recreate localstorage database on invalid schema version.

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

[Marijn Kruisselbrink, 2017-01-11 18:56:59]

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

[commit-bot: I haz the power, 2017-01-11 18:57:30]

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-11 19:06:03]

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

[Marijn Kruisselbrink, 2017-01-11 19:06:20]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-01-11 19:06:52]

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-11 19:10:57]

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

[Marijn Kruisselbrink, 2017-01-11 19:10:58]

Not sure how to best ensure that the database is closed before an attempt is
made to destroy it though...

https://codereview.chromium.org/2625873004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2625873004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/local_storage_context_mojo.cc:332: // TODO(mek):
Somehow ensure closing is processed before Destroy is called.
This TODO is somewhat problematic, since I'm not sure how to accomplish such a
thing. Maybe the Destroy call should be part of the LevelDBDatabase interface
rather than LevelDBService (where it could make sure to close itself before
destroying itself), but then you'd lose the ability to destroy a database
without opening it first. Anything else seems like it will have the problem that
the leveldb service and leveldb database are separate mojo pipes and thus
relying on something on one pipe to have finished before calling a method on the
other pipe is processed is dangerous. Any thoughts?

[commit-bot: I haz the power, 2017-01-11 19:46:04]

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

[commit-bot: I haz the power, 2017-01-11 19:46:05]

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-01-11 19:58:54]

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

[commit-bot: I haz the power, 2017-01-11 19:59:45]

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-11 20:16:58]

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

[commit-bot: I haz the power, 2017-01-11 20:17:01]

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

[jam, 2017-01-12 17:24:42]

https://codereview.chromium.org/2625873004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2625873004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/local_storage_context_mojo.cc:332: // TODO(mek):
Somehow ensure closing is processed before Destroy is called.
On 2017/01/11 19:10:58, Marijn Kruisselbrink wrote:
> This TODO is somewhat problematic, since I'm not sure how to accomplish such a
> thing. Maybe the Destroy call should be part of the LevelDBDatabase interface
> rather than LevelDBService (where it could make sure to close itself before
> destroying itself), but then you'd lose the ability to destroy a database
> without opening it first. Anything else seems like it will have the problem
that
> the leveldb service and leveldb database are separate mojo pipes and thus
> relying on something on one pipe to have finished before calling a method on
the
> other pipe is processed is dangerous. Any thoughts?

If Destroy is on LevelDBInterface, won't there be the same issue for ordering
since the Open is on LevelDBService?

To solve the ordering issue, we could use associated interfaces. wdyt?
https://www.chromium.org/developers/design-documents/mojo/associated-interfaces

https://codereview.chromium.org/2625873004/diff/40001/components/leveldb/publ...
File components/leveldb/public/interfaces/leveldb.mojom (right):

https://codereview.chromium.org/2625873004/diff/40001/components/leveldb/publ...
components/leveldb/public/interfaces/leveldb.mojom:76: string dbname) =>
(DatabaseError status);
nit: indentation

also please add documentation to this method. we should have had some on the few
before but maybe skipped because it seemed obvious.

[Marijn Kruisselbrink, 2017-01-13 19:13:49]

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

[commit-bot: I haz the power, 2017-01-13 19:14: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-01-13 19:25:03]

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

[Marijn Kruisselbrink, 2017-01-13 19:25:11]

https://codereview.chromium.org/2625873004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2625873004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/local_storage_context_mojo.cc:332: // TODO(mek):
Somehow ensure closing is processed before Destroy is called.
On 2017/01/12 at 17:24:42, jam wrote:
> On 2017/01/11 19:10:58, Marijn Kruisselbrink wrote:
> > This TODO is somewhat problematic, since I'm not sure how to accomplish such
a
> > thing. Maybe the Destroy call should be part of the LevelDBDatabase
interface
> > rather than LevelDBService (where it could make sure to close itself before
> > destroying itself), but then you'd lose the ability to destroy a database
> > without opening it first. Anything else seems like it will have the problem
that
> > the leveldb service and leveldb database are separate mojo pipes and thus
> > relying on something on one pipe to have finished before calling a method on
the
> > other pipe is processed is dangerous. Any thoughts?
> 
> If Destroy is on LevelDBInterface, won't there be the same issue for ordering
since the Open is on LevelDBService?
I suppose so, although it seemed less likely that you'd be trying to open a
database while simultaneously trying to destroy it than that you'd want to
destroy a database that you had open already.

> To solve the ordering issue, we could use associated interfaces. wdyt?
https://www.chromium.org/developers/design-documents/mojo/associated-interfaces

Yes, that would work. Changed it to do so. Somehow using associated interface
does kind of always feel like doing it the wrong way to me, with it being a c++
only feature and all (or did that change recently?)...

https://codereview.chromium.org/2625873004/diff/40001/components/leveldb/publ...
File components/leveldb/public/interfaces/leveldb.mojom (right):

https://codereview.chromium.org/2625873004/diff/40001/components/leveldb/publ...
components/leveldb/public/interfaces/leveldb.mojom:76: string dbname) =>
(DatabaseError status);
On 2017/01/12 at 17:24:42, jam wrote:
> nit: indentation
> 
> also please add documentation to this method. we should have had some on the
few before but maybe skipped because it seemed obvious.

Fixed, also added (minimal) documentation to the various Open methods.

[commit-bot: I haz the power, 2017-01-13 19:25:54]

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-13 20:33:52]

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

[commit-bot: I haz the power, 2017-01-13 20:33:53]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-14 00:17:55]

lgtm

https://codereview.chromium.org/2625873004/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/local_storage_context_mojo.cc (right):

https://codereview.chromium.org/2625873004/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/local_storage_context_mojo.cc:332: // TODO(mek):
Somehow ensure closing is processed before Destroy is called.
On 2017/01/13 19:25:11, Marijn Kruisselbrink wrote:
> On 2017/01/12 at 17:24:42, jam wrote:
> > On 2017/01/11 19:10:58, Marijn Kruisselbrink wrote:
> > > This TODO is somewhat problematic, since I'm not sure how to accomplish
such
> a
> > > thing. Maybe the Destroy call should be part of the LevelDBDatabase
> interface
> > > rather than LevelDBService (where it could make sure to close itself
before
> > > destroying itself), but then you'd lose the ability to destroy a database
> > > without opening it first. Anything else seems like it will have the
problem
> that
> > > the leveldb service and leveldb database are separate mojo pipes and thus
> > > relying on something on one pipe to have finished before calling a method
on
> the
> > > other pipe is processed is dangerous. Any thoughts?
> > 
> > If Destroy is on LevelDBInterface, won't there be the same issue for
ordering
> since the Open is on LevelDBService?
> I suppose so, although it seemed less likely that you'd be trying to open a
> database while simultaneously trying to destroy it than that you'd want to
> destroy a database that you had open already.
> 
> > To solve the ordering issue, we could use associated interfaces. wdyt?
>
https://www.chromium.org/developers/design-documents/mojo/associated-interfaces
> 
> Yes, that would work. Changed it to do so. Somehow using associated interface
> does kind of always feel like doing it the wrong way to me, with it being a
c++
> only feature and all (or did that change recently?)...

JavaScript support is being worked on :)

Associated interfaces shouldn't be viewed negatively, it's very useful to split
logically separate methods into separate interfaces, while still maintaining
order. Otherwise things that don't belong together are grouped.

https://codereview.chromium.org/2625873004/diff/80001/components/leveldb/leve...
File components/leveldb/leveldb_service_unittest.cc (right):

https://codereview.chromium.org/2625873004/diff/80001/components/leveldb/leve...
components/leveldb/leveldb_service_unittest.cc:344:
leveldb()->Destroy(std::move(directory), "test", Capture(&error,
run_loop.QuitClosure()));
nit: 80 chars, also below

https://codereview.chromium.org/2625873004/diff/80001/components/leveldb/leve...
components/leveldb/leveldb_service_unittest.cc:353: // Reconnect to the database
shuold fail.
nit: should

[Marijn Kruisselbrink, 2017-01-17 19:33:44]

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

[commit-bot: I haz the power, 2017-01-17 19:34: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-01-17 20:29:18]

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

[commit-bot: I haz the power, 2017-01-17 20:29:19]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2017-01-17 20:47:31]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2017-01-17 20:47:31]

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/2625873004/#ps100001
(title: "80 cols, and fix typo")

[commit-bot: I haz the power, 2017-01-17 20:47:58]

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-17 20:53:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484686051133110,
"parent_rev": "f0d53a35191a6c7e748dd789e85b1ab98e42c2a1", "commit_rev":
"9b1c496fb33eae3e6c647730d1c67444eb3e3925"}

[commit-bot: I haz the power, 2017-01-17 20:57:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484686051133110,
"parent_rev": "f0d53a35191a6c7e748dd789e85b1ab98e42c2a1", "commit_rev":
"9b1c496fb33eae3e6c647730d1c67444eb3e3925"}

[commit-bot: I haz the power, 2017-01-17 21:00:06]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484686051133110,
"parent_rev": "f0d53a35191a6c7e748dd789e85b1ab98e42c2a1", "commit_rev":
"9b1c496fb33eae3e6c647730d1c67444eb3e3925"}

[commit-bot: I haz the power, 2017-01-17 21:08:49]

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

[commit-bot: I haz the power, 2017-01-17 21:08:51]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[Marijn Kruisselbrink, 2017-01-17 21:13:44]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2017-01-17 21:14:13]

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-17 21:20:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484687624102070,
"parent_rev": "064f04c82c2f6cd3e9a77703362f9373034c248a", "commit_rev":
"3cf31809c9d342cfa7c1ae2844bdacf342dd39b9"}

[commit-bot: I haz the power, 2017-01-17 21:21:37]

Description was changed from

==========
Delete and try to recreate localstorage database on invalid schema version.

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

to

==========
Delete and try to recreate localstorage database on invalid schema version.

BUG=586194

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

[commit-bot: I haz the power, 2017-01-17 21:21:38]

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

[Wez, 2017-02-07 22:28:16]

wez@chromium.org changed reviewers:
+ mbarbella@chromium.org

[Wez, 2017-02-07 22:28:16]

Hallo mbarbella@chromium.org!
Due to a depot_tools patch which mistakenly removed the OWNERS check for
non-source files (see crbug.com/684270), the following files landed in this CL
and need a retrospective review from you:
	components/leveldb/public/interfaces/leveldb.mojom
Thanks,
Wez