Bug 88829 – IndexedDB: Add tests, an assert, and some comments around open/close/setVersion call sequencing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

IndexedDB: Add tests, an assert, and some comments

[David Grogan]

Created attachment 146989 [details]
Patch

[David Grogan]

Some tests and comments I added while looking through our pending call queues.

[Joshua Bell]

Comment on attachment 146989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146989&action=review

> Source/WebCore/ChangeLog:3
> +        IndexedDB: Add tests, an assert, and some comments

Be more specific about the domain of the changes, e.g. "around open/close/setVersion call sequencing"

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:291
> +    // If there were any pending set version calls, we better have started one.

Only true if there is only one connection to this database, correct? If that's the case, can we ASSERT on that at the start of the function?

(That's a surprise to me - I thought we called processPendingCalls() more often. Either way, we should be assertive about it.)

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:317
> +    // Given the check above, it appears that calls cannot be requeued by

Add an ASSERT at the end of the function that m_pendingOpenCalls is empty?

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions-expected.txt:74
> +PASS self.steps.toString() is "h.open,h.open.onsuccess,h2.open,h2.open.onsuccess,h.setVersion,deleteDatabase(),h2.onversionchange,h.setVersion.onblocked,h3.open,h2.close,h.setVersion.onsuccess,h.setVersion.transaction-complete,h.onversionchange,deleteDatabase().onblocked,h.close,deleteDatabase().onsuccess,h3.open.onsuccess"

Feel free to rewrite this test if it'll improve comprehensibility. (It seemed like a good idea at the time, but looks a little "too clever" to me now.)

[David Grogan]

Comment on attachment 146989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146989&action=review

Added a few ASSERTS.  Those invariants might change shortly, but awareness of what changes is the point here.

>> Source/WebCore/ChangeLog:3
>> +        IndexedDB: Add tests, an assert, and some comments
> 
> Be more specific about the domain of the changes, e.g. "around open/close/setVersion call sequencing"

Done.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:291
>> +    // If there were any pending set version calls, we better have started one.
> 
> Only true if there is only one connection to this database, correct? If that's the case, can we ASSERT on that at the start of the function?
> 
> (That's a surprise to me - I thought we called processPendingCalls() more often. Either way, we should be assertive about it.)

We call processPendingCalls when the last or second to last connection has closed and when a version change transaction has finished.  So yeah, we only call it when there is at most one remaining connection, good catch.  ASSERT added.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:317
>> +    // Given the check above, it appears that calls cannot be requeued by
> 
> Add an ASSERT at the end of the function that m_pendingOpenCalls is empty?

Done.

[David Grogan]

Created attachment 147189 [details]
Patch

[David Grogan]

Created attachment 147192 [details]
Patch

[Joshua Bell]

Comment on attachment 147192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147192&action=review

> LayoutTests/storage/indexeddb/resources/factory-deletedatabase-interactions.js:20
> +                debug("    in versionchange, old = " + e.target.version + " new = " + e.version);

Really minor, but maybe use JSON.stringify(e.target.version) and JSON.stringify(e.version) so they're quoted when strings? (The output currently looks a bit odd when there's no old version.)

[David Grogan]

Comment on attachment 147192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147192&action=review

>> LayoutTests/storage/indexeddb/resources/factory-deletedatabase-interactions.js:20
>> +                debug("    in versionchange, old = " + e.target.version + " new = " + e.version);
> 
> Really minor, but maybe use JSON.stringify(e.target.version) and JSON.stringify(e.version) so they're quoted when strings? (The output currently looks a bit odd when there's no old version.)

SG.

[David Grogan]

Created attachment 147200 [details]
Patch

[David Grogan]

Tony, could you review this?

[Tony Chang]

Comment on attachment 147200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147200&action=review

> LayoutTests/storage/indexeddb/resources/three-setversion-calls.js:48
> +      if (i == 0) {

Nit: if (!i) would be WebKit style.

[David Grogan]

Comment on attachment 147200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147200&action=review

>> LayoutTests/storage/indexeddb/resources/three-setversion-calls.js:48
>> +      if (i == 0) {
> 
> Nit: if (!i) would be WebKit style.

Thanks, fixed.

[David Grogan]

Created attachment 147348 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 147348 [details]
Patch for landing

Clearing flags on attachment: 147348

Committed r120222: <http://trac.webkit.org/changeset/120222>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.