IndexedDB: Defer delete calls when there is a running upgrade

IndexedDB: Defer delete calls when there is a running upgrade

Database open requests enter a limbo state when there is an upgrade
running - the request neither proceeds nor blocks, since there is no
successfully opened request to receive the "versionchange" event.
The same should be true for database delete requests, otherwise
either the delete happens during the upgrade (bad!) or is blocked but
the open connection is unaware of the request and can't close itself
(also bad!).

The spec is ambiguous here [1][2]; this change aligns Chrome's
behavior with Firefox.

[1] https://github.com/w3c/IndexedDB/issues/78
[2] https://github.com/w3c/IndexedDB/issues/79

R=palakj@chromium.org,cmumford@chromium.org
BUG=617963
Committed: https://crrev.com/dd17e5bea9d450f71441c2c8694d910ddfc9df1a
Cr-Commit-Position: refs/heads/master@{#404536}

[jsbell, 2016-06-22 22:45:34]

palakj@ - can you please take a look?

Thanks!

[cmumford, 2016-06-23 21:44:20]

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1714: return
IsUpgradePendingOrRunning() || !blocked_delete_calls_.empty();
nit: blocked_delete_calls_.empty() is constant time and faster - check it first?

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1966:
!blocked_delete_calls_.size()) {
Nit: I know you're sticking with existing pattern, but
blocked_delete_calls_.empty() is more readable, and std::list::size() might not
be constant time.

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.h:341: PendingDeleteCallList
blocked_delete_calls_;
Why do we use a std::list instead of std::queue?

[jsbell, 2016-06-23 22:26:50]

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1714: return
IsUpgradePendingOrRunning() || !blocked_delete_calls_.empty();
IsUpgradePendingOrRunning() should be constant time too - at most one
transaction needs to be checked for IsUpgradeRunning().

But yeah, I can flip it.

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1966:
!blocked_delete_calls_.size()) {
On 2016/06/23 21:44:19, cmumford wrote:
> Nit: I know you're sticking with existing pattern, but
> blocked_delete_calls_.empty() is more readable, and std::list::size() might
not
> be constant time.

Whoops, yeah, I'll use empty(). That was ported from webkit code. :P

(std::list::size() is apparently constant since C++11)

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.h:341: PendingDeleteCallList
blocked_delete_calls_;
On 2016/06/23 21:44:20, cmumford wrote:
> Why do we use a std::list instead of std::queue?

No good reason; probably ported mechanically from WebKit. I'll convert these to
queues.

[jsbell, 2016-06-23 23:00:27]

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.h:341: PendingDeleteCallList
blocked_delete_calls_;
On 2016/06/23 22:26:50, jsbell wrote:
> On 2016/06/23 21:44:20, cmumford wrote:
> > Why do we use a std::list instead of std::queue?
> 
> No good reason; probably ported mechanically from WebKit. I'll convert these
to
> queues.

Ah, I remember. you can't iterate over a std::queue, and we iterate over these
when sending out OnBlocked().

That isn't done for pending_delete_calls_ so we could switch that to be a queue.

[palakj1, 2016-06-24 03:32:52]

On 2016/06/22 at 22:45:34, jsbell wrote:
> palakj@ - can you please take a look?
> 
> Thanks!

Is it a spec rule that pending delete calls are handled before pending open
calls?
So, in case of
indexedDB.open(db)
indexedDB.open(db)
indexedDB.deleteDatabase(db)

we expect the sequence: open, delete, open.

Similarly, delete call registered at upgradeneeded is triggered first and then
delete call registered at success.
Is that the expected behaviour? Do we need to have a test to assert this order.

[palakj1, 2016-06-24 03:41:40]

palakj@google.com changed reviewers:
+ palakj@google.com

[palakj1, 2016-06-24 03:41:41]

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/2084053004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.h:341: PendingDeleteCallList
blocked_delete_calls_;
nit: the names pending_delete_calls_ and blocked_delete_calls_ are a bit
confusing regarding their purpose, although the comments clarify things.

[cmumford, 2016-06-24 16:15:01]

lgtm

[jsbell, 2016-06-24 20:48:42]

On 2016/06/24 03:32:52, palakj1 wrote:
> Is it a spec rule that pending delete calls are handled before pending open
> calls?
> So, in case of
> indexedDB.open(db)
> indexedDB.open(db)
> indexedDB.deleteDatabase(db)
> 
> we expect the sequence: open, delete, open.

The spec does define behavior here (which chrome follows) but that's not
necessarily desired. We've discussed this in the past but pre-github.
 I filed:

https://github.com/w3c/IndexedDB/issues/79

This is covered by storage/indexeddb/database-deletepending-flag.html:

> Similarly, delete call registered at upgradeneeded is triggered first and then
> delete call registered at success.
> Is that the expected behaviour? Do we need to have a test to assert this
order.

I'll add a test case for this.

[palakj1, 2016-06-24 21:36:19]

lgtm

https://codereview.chromium.org/2084053004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/empty-transaction-order.html
(left):

https://codereview.chromium.org/2084053004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/empty-transaction-order.html:8:
function expect(t, expected) {
why this test here?

[jsbell, 2016-06-24 21:37:28]

https://codereview.chromium.org/2084053004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/empty-transaction-order.html
(left):

https://codereview.chromium.org/2084053004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/empty-transaction-order.html:8:
function expect(t, expected) {
On 2016/06/24 21:36:19, palakj1 wrote:
> why this test here?

I moved this into testharness-helpers.js since it was handy for other tests.

[palakj, 2016-06-28 22:20:36]

lgtm

[jsbell, 2016-07-07 17:13:39]

FYI, after talking w/ other implementers, there's probably additional work to do
here sequencing opens/deletes to align implementations. (Thanks for pushing,
palak@!)

My plan is to land this (after rebasing) since it still seems like a worthwhile
intermediate step.

[jsbell, 2016-07-08 22:22:02]

Description was changed from

==========
IndexedDB: Defer delete calls when there is a running upgrade

The database open requests enter a limbo state when there is an
upgrade running - the request neither proceeds nor blocks, since there
is no successfully opened request to receive the "versionchange"
event. The same should be true for database delete requests, otherwise
either the delete happens during the upgrade (bad!) or is blocked but
the open connection is unaware of the request and can't close itself
(also bad!).

The spec is ambiguous here [1]; this change aligns Chrome's behavior
with Firefox.

[1] https://github.com/w3c/IndexedDB/issues/78

R=palakj@chromium.org,cmumford@chromium.org
BUG=617963
==========

to

==========
IndexedDB: Defer delete calls when there is a running upgrade

Database open requests enter a limbo state when there is an upgrade
running - the request neither proceeds nor blocks, since there is no
successfully opened request to receive the "versionchange" event.
The same should be true for database delete requests, otherwise
either the delete happens during the upgrade (bad!) or is blocked but
the open connection is unaware of the request and can't close itself
(also bad!).

The spec is ambiguous here [1][2]; this change aligns Chrome's
behavior with Firefox.

[1] https://github.com/w3c/IndexedDB/issues/78
[2] https://github.com/w3c/IndexedDB/issues/79

R=palakj@chromium.org,cmumford@chromium.org
BUG=617963
==========

[jsbell, 2016-07-08 22:34:04]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-07-08 22:34:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org, palakj@google.com, palakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2084053004/#ps40001
(title: "Rebased, made delete lists contain unique_ptrs")

[commit-bot: I haz the power, 2016-07-08 22:34:24]

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, 2016-07-08 23:50:24]

Description was changed from

==========
IndexedDB: Defer delete calls when there is a running upgrade

Database open requests enter a limbo state when there is an upgrade
running - the request neither proceeds nor blocks, since there is no
successfully opened request to receive the "versionchange" event.
The same should be true for database delete requests, otherwise
either the delete happens during the upgrade (bad!) or is blocked but
the open connection is unaware of the request and can't close itself
(also bad!).

The spec is ambiguous here [1][2]; this change aligns Chrome's
behavior with Firefox.

[1] https://github.com/w3c/IndexedDB/issues/78
[2] https://github.com/w3c/IndexedDB/issues/79

R=palakj@chromium.org,cmumford@chromium.org
BUG=617963
==========

to

==========
IndexedDB: Defer delete calls when there is a running upgrade

Database open requests enter a limbo state when there is an upgrade
running - the request neither proceeds nor blocks, since there is no
successfully opened request to receive the "versionchange" event.
The same should be true for database delete requests, otherwise
either the delete happens during the upgrade (bad!) or is blocked but
the open connection is unaware of the request and can't close itself
(also bad!).

The spec is ambiguous here [1][2]; this change aligns Chrome's
behavior with Firefox.

[1] https://github.com/w3c/IndexedDB/issues/78
[2] https://github.com/w3c/IndexedDB/issues/79

R=palakj@chromium.org,cmumford@chromium.org
BUG=617963
==========

[commit-bot: I haz the power, 2016-07-08 23:50:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-08 23:50:39]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 23:52:16]

Description was changed from

==========
IndexedDB: Defer delete calls when there is a running upgrade

Database open requests enter a limbo state when there is an upgrade
running - the request neither proceeds nor blocks, since there is no
successfully opened request to receive the "versionchange" event.
The same should be true for database delete requests, otherwise
either the delete happens during the upgrade (bad!) or is blocked but
the open connection is unaware of the request and can't close itself
(also bad!).

The spec is ambiguous here [1][2]; this change aligns Chrome's
behavior with Firefox.

[1] https://github.com/w3c/IndexedDB/issues/78
[2] https://github.com/w3c/IndexedDB/issues/79

R=palakj@chromium.org,cmumford@chromium.org
BUG=617963
==========

to

==========
IndexedDB: Defer delete calls when there is a running upgrade

Database open requests enter a limbo state when there is an upgrade
running - the request neither proceeds nor blocks, since there is no
successfully opened request to receive the "versionchange" event.
The same should be true for database delete requests, otherwise
either the delete happens during the upgrade (bad!) or is blocked but
the open connection is unaware of the request and can't close itself
(also bad!).

The spec is ambiguous here [1][2]; this change aligns Chrome's
behavior with Firefox.

[1] https://github.com/w3c/IndexedDB/issues/78
[2] https://github.com/w3c/IndexedDB/issues/79

R=palakj@chromium.org,cmumford@chromium.org
BUG=617963

Committed: https://crrev.com/dd17e5bea9d450f71441c2c8694d910ddfc9df1a
Cr-Commit-Position: refs/heads/master@{#404536}
==========

[commit-bot: I haz the power, 2016-07-08 23:52:17]

Patchset 3 (id:??) landed as
https://crrev.com/dd17e5bea9d450f71441c2c8694d910ddfc9df1a
Cr-Commit-Position: refs/heads/master@{#404536}