Bug 105559 – IndexedDB: IDBOpenDBRequest sometimes receives two blocked events

• Component: New Bugs
• Status: RESOLVED CONFIGURATION CHANGED

[Joshua Bell]

IndexedDB: IDBOpenDBRequest sometimes receives two blocked events

[Joshua Bell]

Created attachment 180376 [details]
Repro layout test

[Joshua Bell]

Derived a specific layout test for this case from the steps in http://wkbug.com/102713

Fix should be similar to http://wkrev.com/138268

[Joshua Bell]

Created attachment 180444 [details]
Patch

[Joshua Bell]

Just a snapshot - I haven't tried this in content_shell or looked at other browsers yet.

[Joshua Bell]

I think we can merge openConnection and openConnectionWithVersion - I may attempt to do that first and then rebase this patch.

[Joshua Bell]

Yeah, will rebase this on top of wkbug.com/105658

[Joshua Bell]

Created attachment 180572 [details]
Patch

[Joshua Bell]

Rebased on top of http://wkrev.com/138400 - not broadly tested yet, though.

[Joshua Bell]

Our behavior (both with and without this patch) differs from both FF and IE. I reached out to public-webapps but no responses yet (presumably due to holidays).

[Joshua Bell]

dgrogan@, alecflett@ - pending feedback from other implementers, can you take a look?

If the consensus is to match what other browsers do then I think we'd still want something based off this patch, but either make delete two-phase (like open) w/ the first phase that silently-waits followed by a second phase that does versionchange/blocked events. So I may push ahead with this. (I'll re-ping public-webapps on Monday when most folks should have recovered from the holidays.)

[David Grogan]

Comment on attachment 180572 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:447
> +    if (!m_pendingDeleteCalls.isEmpty() && !isDeleteDatabaseBlocked()) {

The first clause of this condition isn't necessary, is it? Feel free to leave it in if you think it improves readability, though.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:450
> +            deleteDatabaseFinal(pendingDeleteCall->callbacks());

It looks like each pending delete call after the first has no effect, correct? But this is the simplest and clearest way to process them all.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:457
> +        ASSERT(m_pendingRunVersionChangeTransactionCalls.size() == 1);

I can't convince myself that this assert is right. If there's an open connection with version 1 and script calls open("db", 2); open("db", 3), won't there be two entries?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:582
> +        if (connectionCount() > 1)

You can remove this line.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:667
> +    if (connectionCount() > 2)

Your comment seems to indicate that this should be >= 2.

> LayoutTests/storage/indexeddb/intversion-long-queue-expected.txt:64
> +FAIL event.oldVersion should be 1 (of type number). Was undefined (of type undefined).

I think you've fixed these since you wrote this patch?

[Joshua Bell]

Comment on attachment 180572 [details]
Patch

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

Thanks for taking a look!

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:447
>> +    if (!m_pendingDeleteCalls.isEmpty() && !isDeleteDatabaseBlocked()) {
> 
> The first clause of this condition isn't necessary, is it? Feel free to leave it in if you think it improves readability, though.

Correct; I'll remove it.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:450
>> +            deleteDatabaseFinal(pendingDeleteCall->callbacks());
> 
> It looks like each pending delete call after the first has no effect, correct? But this is the simplest and clearest way to process them all.

They all get callbacks->onSuccess() called in deleteDatabaseFinal(). So... yeah, this seems clearest to me.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:457
>> +        ASSERT(m_pendingRunVersionChangeTransactionCalls.size() == 1);
> 
> I can't convince myself that this assert is right. If there's an open connection with version 1 and script calls open("db", 2); open("db", 3), won't there be two entries?

In that case there should be multiple entries in m_pendingOpenCalls but at most one of those should have gotten into the upgradeneeded phase.

Does that make sense? (I confess it's been a while since I looked at this patch myself.)

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:582
>> +        if (connectionCount() > 1)
> 
> You can remove this line.

Ah yes, thanks.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:667
>> +    if (connectionCount() > 2)
> 
> Your comment seems to indicate that this should be >= 2.

Ooops. I'll try and come up with a test that gets stuck on this invalid test before correcting it.

>> LayoutTests/storage/indexeddb/intversion-long-queue-expected.txt:64
>> +FAIL event.oldVersion should be 1 (of type number). Was undefined (of type undefined).
> 
> I think you've fixed these since you wrote this patch?

Yep. Will need rebasing.

[Joshua Bell]

Created attachment 187937 [details]
Patch

[Joshua Bell]

Updated patch with feedback from comments and rebased.

(In reply to comment #12)
> 
> >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:457
> >> +        ASSERT(m_pendingRunVersionChangeTransactionCalls.size() == 1);
> > 
> > I can't convince myself that this assert is right. If there's an open connection with version 1 and script calls open("db", 2); open("db", 3), won't there be two entries?
> 
> In that case there should be multiple entries in m_pendingOpenCalls but at most one of those should have gotten into the upgradeneeded phase.
> 
> Does that make sense? (I confess it's been a while since I looked at this patch myself.)

dgrogan@ - No change to the patch here - can you take another look, though?

> >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:667
> >> +    if (connectionCount() > 2)
> > 
> > Your comment seems to indicate that this should be >= 2.
> 
> Ooops. I'll try and come up with a test that gets stuck on this invalid test before correcting it.

Fixed, but the previous code just would have resulted in extra no-op calls to processPendingCalls() if connectionCount() == 2 so the behavior can't be distinguished.

[Joshua Bell]

(In reply to comment #9)
> Our behavior (both with and without this patch) differs from both FF and IE. I reached out to public-webapps but no responses yet (presumably due to holidays).

FYI, sounds like Firefox considers their behavior buggy. No word from IE.

[Joshua Bell]

(In reply to comment #14)
> Updated patch with feedback from comments and rebased.
> 
> dgrogan@ - can you take another look, though?

ping?

[Joshua Bell]

Created attachment 192552 [details]
Patch

[Joshua Bell]

Rebased.

dgrogan@ - can you take a look? See response to questions above.

[Build Bot]

Comment on attachment 192552 [details]
Patch

Attachment 192552 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17161445

New failing tests:
editing/selection/selection-modify-crash.html

[Joshua Bell]

(In reply to comment #19)
> New failing tests:
> editing/selection/selection-modify-crash.html

I'm guessing that's unrelated. :)

[Joshua Bell]

Created attachment 196011 [details]
Patch

[Joshua Bell]

(In reply to comment #21)
> Created an attachment (id=196011) [details]
> Patch

Just a rebase.

[Levi Weintraub]

Should this be moved to blink? If so, please clear the flags on the patch and mark this bug as invalid.

[Joshua Bell]

I've copied this to blink. The issue/patch is still valid for WebKit. I'm pinging possible new owners.

[Radar WebKit Bug Importer]

<rdar://problem/96844951>

[Sihui Liu]

We've refactored the code a lot since the bug was fired, and we won't fire two blocked events on one IDBOpenDBRequest nowadays, see ServerOpenDBRequest::maybeNotifyRequestBlocked.

Also the test LayoutTests/storage/indexeddb/double-blocked-on-open.html added in the patch does not conform to the spec nowadays: new request won't be processed until the previous requests are completed, 
see https://www.w3.org/TR/IndexedDB/#connection-queues and https://www.w3.org/TR/IndexedDB/#opening.

So I will close this bug.