Bug 69307 – make IDBFactory.open wait for pending setVersion transactions to complete

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

make IDBFactory.open wait for pending setVersion transactions to complete

[David Grogan]

Created attachment 109553 [details]
unfinished patch

[Joshua Bell]

Created attachment 109700 [details]
Patch

[Joshua Bell]

Can you take an initial peek at the patch? 

* This is based on initial code by dgrogan, with additions to ensure that when open() is called during a running VERSION_CHANGE transaction it becomes a pending action as well.

* Tests are pending. I have a test page, just need to morph it into a proper layouttest.

* This adds new public methods to IDBDatabaseBackendImpl (transactionStarted/Finished) and explicit calls from the transaction. This is simple, but seems limited. Another approach I considered was to add two more callback queues to transaction, one for started callbacks and one for finished callbacks. This is much heavier weight, but would allow the database to selectively subscribe to only VERSION_CHANGE transaction start/finish states, and possibly other uses. However, since the version change mechanism is changing soon this seems premature. 

* It feels... odd that both the front end IDBDatabase and back end need to keep track of the active version change transactions. I worry they could get out of sync.

[Joshua Bell]

Created attachment 109712 [details]
Patch

[Joshua Bell]

Latest patch adds tests.

[Joshua Bell]

... but don't look too closely at the details, other tests turn up a crash. :P

[Joshua Bell]

Created attachment 109713 [details]
Patch

[Joshua Bell]

The source of the crash (fixed in latest patch) was an assertion failure (yay!) firing when calling setVersion was called inside a VERSION_CHANGE transaction. The new patch now defers this, but the spec is hazy on exactly what should happen . Since that is being obsoleted by the new versioning mechanism, this approach seems acceptable.

[WebKit Review Bot]

Comment on attachment 109713 [details]
Patch

Attachment 109713 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9959014

New failing tests:
inspector/debugger/debugger-activation-crash2.html

[Hans Wennborg]

Comment on attachment 109713 [details]
Patch

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

> LayoutTests/storage/indexeddb/version-change-exclusive.html:67
> +	        debug("ending work in VERSION_CHANGE transaction");

nit: indentation

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:251
> +        // FIXME: if setVersion returns w/o initiating a transaction, pending opens will never run

This sounds like a problem?

[Joshua Bell]

Created attachment 109852 [details]
Patch

[David Grogan]

Comment on attachment 109852 [details]
Patch

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

Could you come up with a gnarly layout test that interleaves some opens, closes and setversions?  Anything to get the new code in this subtle area to as close to 100% test-covered as possible.  (Or is it already?)

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:187
> +        processPendingCalls();

Does your new layout test fail without this line or does an existing one?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:249
> +    if (!m_pendingSetVersionCalls.isEmpty()) {

This "if" doesn't have to be "while" because processPendingCalls is called again when the first pending setVersion call is called, is that right?  Maybe that means that the while on line old 232 / new 297 should be an if also?  Maybe that while loop can just be replaced with a call to processPendingCalls()?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:258
> +      RefPtr<PendingOpenCall> pendingOpenCall = m_pendingOpenCalls.takeFirst();

this should be indented 4 spaces.  (apologies, that was probably from me)

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:281
> +void IDBDatabaseBackendImpl::openEventually(PassRefPtr<IDBCallbacks> callbacks)

I think I named both open and openEventually, but they are confusing.  open's job is just to give the backend some callbacks that are created after openEventually succeeds.  Could you take a stab at renaming one or both of these?

[Joshua Bell]

Comment on attachment 109852 [details]
Patch

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

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:187
>> +        processPendingCalls();
> 
> Does your new layout test fail without this line or does an existing one?

Nope and nope. I'll see if I can come up with something, now that it seems like this CL is on the right track.

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:249
>> +    if (!m_pendingSetVersionCalls.isEmpty()) {
> 
> This "if" doesn't have to be "while" because processPendingCalls is called again when the first pending setVersion call is called, is that right?  Maybe that means that the while on line old 232 / new 297 should be an if also?  Maybe that while loop can just be replaced with a call to processPendingCalls()?

Re: "doesn't have to be a while" - exactly.

And... yes, good catch.

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:281
>> +void IDBDatabaseBackendImpl::openEventually(PassRefPtr<IDBCallbacks> callbacks)
> 
> I think I named both open and openEventually, but they are confusing.  open's job is just to give the backend some callbacks that are created after openEventually succeeds.  Could you take a stab at renaming one or both of these?

I'll ponder.

[Joshua Bell]

Created attachment 110227 [details]
Patch

[Joshua Bell]

New patch that includes more fun test permutations. It caught one bug in the implementation.

[David Grogan]

Comment on attachment 110227 [details]
Patch

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

I think the only substantive issue is the potential ref-counting cycle.  The rest of the comments are style stuff or making sure I understand what's going on.

Hopefully most of this can be reused when we switch to the new open/version API.

> LayoutTests/storage/indexeddb/open-close-version-expected.txt:78
> +'h2.open.onsuccess'

I think these last two would be in this same order before your patch because h1.setVersion.onsuccess is printed before all the async puts.  But if you passed in something to h1.setVersion, like h1.setVersion({onsuccess: function() { debug ("something") }}), then the order of "something" and "h2.open.onsuccess" would be different pre-patch vs post-patch.

> LayoutTests/storage/indexeddb/open-close-version.html:95
> +    run([function(c) { h1.open({onsuccess: c}); },

c stands for commands?

> LayoutTests/storage/indexeddb/open-close-version.html:99
> +                                    setTimeout(h2.close.bind(h2), 0);

without bind, h2.close would try to call test1.handle.close() instead of h2.handle.close()?

> LayoutTests/storage/indexeddb/open-close-version.html:183
> +                               "h2.setVersion.onerror",

This is the ABORT_ERR from IDBDatabaseBackendImpl.cpp line new186?

> LayoutTests/storage/indexeddb/open-close-version.html:202
> +         function(c) { h2.close(); c(); },

IIUC, explicitly calling c() runs the next command immediately.  If so, the c() on the last command is a no-op?

> LayoutTests/storage/indexeddb/open-close-version.html:214
> +                               "h1.setVersion.onblocked",

This onblocked should go away when we fix the FIXME that causes test2 to fail, right?

> LayoutTests/storage/indexeddb/open-close-version.html:229
> +    run([function(c) { h1.open({onsuccess: c}); },

It seems that this scenario is covered by your other test, version-change-exclusive?  Is there something different that I missed?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:231
> +    if (prpTransaction->mode() == IDBTransaction::VERSION_CHANGE) {

It won't make any difference in this function, but it's common practice to not use prp parameters for anything except assigning or passing through.

(You've probably already seen it, but in case not, there's a long discussion on RefPtr and PassRefPtr at http://www.webkit.org/coding/RefPtr.html)

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:241
> +        m_runningVersionChangeTransaction = 0;

I think the conventional idiom here is to use .clear() insted of = 0.  Though I'm less certain about this.  And I think I use = 0 :-[

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:250
> +    m_pendingSetVersionCalls.swap(pendingSetVersionCalls);

You put this swap in here to avoid an infinite loop where we would just keep popping and adding the same setVersion call?  How did the old code not fall into that trap?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:251
> +    while (!pendingSetVersionCalls.isEmpty()) {

Using a while here instead of an if lets you avoid putting a processPendingCalls() where ABORT_ERR is fired on line 186, right?  And using a while is needed because of the swap.

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:105
> +    RefPtr<IDBTransactionBackendInterface> m_runningVersionChangeTransaction;

Do you know if there are potential ref-counting cycles between IDBTransactionBackendImpl and IDBDatabaseBackendImpl?  Off the top of my head I think this is fine but am not sure.

[Joshua Bell]

Comment on attachment 110227 [details]
Patch

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

>> LayoutTests/storage/indexeddb/open-close-version-expected.txt:78
>> +'h2.open.onsuccess'
> 
> I think these last two would be in this same order before your patch because h1.setVersion.onsuccess is printed before all the async puts.  But if you passed in something to h1.setVersion, like h1.setVersion({onsuccess: function() { debug ("something") }}), then the order of "something" and "h2.open.onsuccess" would be different pre-patch vs post-patch.

You're right; I think this predates some test refactoring. I'll make the "after all the async puts" step always add something to the list of steps so this would show up.

>> LayoutTests/storage/indexeddb/open-close-version.html:95
>> +    run([function(c) { h1.open({onsuccess: c}); },
> 
> c stands for commands?

"continuation", i.e. go on to the next step in the sequence passed to run(). I'll brainstorm on better naming, and/or unwrapping all of this stuff

>> LayoutTests/storage/indexeddb/open-close-version.html:99
>> +                                    setTimeout(h2.close.bind(h2), 0);
> 
> without bind, h2.close would try to call test1.handle.close() instead of h2.handle.close()?

h2.close.bind(h2) is a shortcut for function() { h2.close(); } that was standardized in ES5. I'll switch to the explicit-function version because I agree it's clearer.

>> LayoutTests/storage/indexeddb/open-close-version.html:202
>> +         function(c) { h2.close(); c(); },
> 
> IIUC, explicitly calling c() runs the next command immediately.  If so, the c() on the last command is a no-op?

True, that's a leftover.

>> LayoutTests/storage/indexeddb/open-close-version.html:214
>> +                               "h1.setVersion.onblocked",
> 
> This onblocked should go away when we fix the FIXME that causes test2 to fail, right?

Correct. I'll remove it here as well and mark this as expected to FAIL.

>> LayoutTests/storage/indexeddb/open-close-version.html:229
>> +    run([function(c) { h1.open({onsuccess: c}); },
> 
> It seems that this scenario is covered by your other test, version-change-exclusive?  Is there something different that I missed?

Nope, that's correct. Should one or the other be eliminated? version-change-exclusive is very straightforward. This test file has the infrastructure for verifying more elaborate sequences with less per-test code.

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:250
>> +    m_pendingSetVersionCalls.swap(pendingSetVersionCalls);
> 
> You put this swap in here to avoid an infinite loop where we would just keep popping and adding the same setVersion call?  How did the old code not fall into that trap?

Correct. And... the old code would not try to loop over the pending calls if there was more than one open connection (line 295, in close()).  This doesn't change in new code in the close() case, although it's not strictly necessary - if there are multiple connections the calls will get requeued even if processPendingCalls() ran. 

(I'm unsure of the behavior if setVersion is called more than once from the same connection, and will dig further.)

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:251
>> +    while (!pendingSetVersionCalls.isEmpty()) {
> 
> Using a while here instead of an if lets you avoid putting a processPendingCalls() where ABORT_ERR is fired on line 186, right?  And using a while is needed because of the swap.

Correct. Calling processPendingCalls() from inside setVersion() (line 186) would process the list by recursion, but could end up in an infinite loop when there was a setVersion that was deferred again.

>> Source/WebCore/storage/IDBDatabaseBackendImpl.h:105
>> +    RefPtr<IDBTransactionBackendInterface> m_runningVersionChangeTransaction;
> 
> Do you know if there are potential ref-counting cycles between IDBTransactionBackendImpl and IDBDatabaseBackendImpl?  Off the top of my head I think this is fine but am not sure.

There is already a cycle of IDBDatabaseBackendImpl -(has a)-> IDBTransactionCoordinator -(references)->IDBTransaction -(references)-> IDBDatabaseBackendImpl, so I don't think this adds one. If we can stop a transaction without aborting, that would leak.

[David Grogan]

Comment on attachment 110227 [details]
Patch

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

Thanks for digging into this.

>>> LayoutTests/storage/indexeddb/open-close-version.html:229
>>> +    run([function(c) { h1.open({onsuccess: c}); },
>> 
>> It seems that this scenario is covered by your other test, version-change-exclusive?  Is there something different that I missed?
> 
> Nope, that's correct. Should one or the other be eliminated? version-change-exclusive is very straightforward. This test file has the infrastructure for verifying more elaborate sequences with less per-test code.

We can (and should) keep both.  I just wanted to ensure I wasn't missing anything.

>>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:250
>>> +    m_pendingSetVersionCalls.swap(pendingSetVersionCalls);
>> 
>> You put this swap in here to avoid an infinite loop where we would just keep popping and adding the same setVersion call?  How did the old code not fall into that trap?
> 
> Correct. And... the old code would not try to loop over the pending calls if there was more than one open connection (line 295, in close()).  This doesn't change in new code in the close() case, although it's not strictly necessary - if there are multiple connections the calls will get requeued even if processPendingCalls() ran. 
> 
> (I'm unsure of the behavior if setVersion is called more than once from the same connection, and will dig further.)

Ah I see, the code would only loop over multiple pending setVersion calls if some of the connections associated with those setVersion calls had closed, in which case they wouldn't be requeue'd.  And good call on checking what happens if setVersion is called more than once from the same connection.

>>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:251
>>> +    while (!pendingSetVersionCalls.isEmpty()) {
>> 
>> Using a while here instead of an if lets you avoid putting a processPendingCalls() where ABORT_ERR is fired on line 186, right?  And using a while is needed because of the swap.
> 
> Correct. Calling processPendingCalls() from inside setVersion() (line 186) would process the list by recursion, but could end up in an infinite loop when there was a setVersion that was deferred again.

I just convinced myself that the line295 check in close would prevent a setversion from being deferred again.  Is that wrong?

[Joshua Bell]

Comment on attachment 110227 [details]
Patch

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

>>>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:251
>>>> +    while (!pendingSetVersionCalls.isEmpty()) {
>>> 
>>> Using a while here instead of an if lets you avoid putting a processPendingCalls() where ABORT_ERR is fired on line 186, right?  And using a while is needed because of the swap.
>> 
>> Correct. Calling processPendingCalls() from inside setVersion() (line 186) would process the list by recursion, but could end up in an infinite loop when there was a setVersion that was deferred again.
> 
> I just convinced myself that the line295 check in close would prevent a setversion from being deferred again.  Is that wrong?

Not wrong, but that only applies from close(). If processPendingCalls() is called from transactionFinished() it could loop.

[Joshua Bell]

Created attachment 110763 [details]
Patch

[Joshua Bell]

Created attachment 110775 [details]
Patch

[David Grogan]

Comment on attachment 110775 [details]
Patch

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

LGTM

> LayoutTests/storage/indexeddb/open-close-version-expected.txt:37
> +NOTE: Will FAIL with extra bogus h1.setVersion.onblocked step; there's a FIXME

I just filed crbug.com/100123.  Can you add a link to it in the FIXME in the code and replace these references to the FIXME with references to the bug?

> LayoutTests/storage/indexeddb/open-close-version.html:290
> +        test7();

should this just be done()?

[Joshua Bell]

Created attachment 110900 [details]
Patch

[Joshua Bell]

Comment on attachment 110900 [details]
Patch

Incorporating last bit of feedback - comment changes, and removing an empty test.

[Tony Chang]

Comment on attachment 110900 [details]
Patch

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

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:58
> +    // FIXME: rename "open" to something more descriptive, like registerFrontEndCallbacks

Nit: Capitalize first letter and end in a period.

[Joshua Bell]

Created attachment 110907 [details]
Patch

[Tony Chang]

Comment on attachment 110907 [details]
Patch

Protip: If you already have r+, you can use "webkit-patch land-safely" to upload a new diff iwth cq+ .

[WebKit Review Bot]

Comment on attachment 110907 [details]
Patch

Clearing flags on attachment: 110907

Committed r97417: <http://trac.webkit.org/changeset/97417>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.