IndexedDB: IDBOpenDBRequest sometimes receives two blocked events
Created attachment 180376 [details] Repro layout test
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
Created attachment 180444 [details] Patch
Just a snapshot - I haven't tried this in content_shell or looked at other browsers yet.
I think we can merge openConnection and openConnectionWithVersion - I may attempt to do that first and then rebase this patch.
Yeah, will rebase this on top of wkbug.com/105658
Created attachment 180572 [details] Patch
Rebased on top of http://wkrev.com/138400 - not broadly tested yet, though.
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).
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.)
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?
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.
Created attachment 187937 [details] Patch
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.
(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.
(In reply to comment #14) > Updated patch with feedback from comments and rebased. > > dgrogan@ - can you take another look, though? ping?
Created attachment 192552 [details] Patch
Rebased. dgrogan@ - can you take a look? See response to questions above.
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
(In reply to comment #19) > New failing tests: > editing/selection/selection-modify-crash.html I'm guessing that's unrelated. :)
Created attachment 196011 [details] Patch
(In reply to comment #21) > Created an attachment (id=196011) [details] > Patch Just a rebase.
Should this be moved to blink? If so, please clear the flags on the patch and mark this bug as invalid.
I've copied this to blink. The issue/patch is still valid for WebKit. I'm pinging possible new owners.
<rdar://problem/96844951>
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.