Bug 92674 – IndexedDB: Layout test showing delete database getting two blocked events

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Grogan]

Layout test showing delete database getting two receive events

[David Grogan]

Created attachment 155350 [details]
Patch

[David Grogan]

Created attachment 155351 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155351 [details]
Patch

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

New failing tests:
storage/indexeddb/dont-fire-blocked-twice.html

[WebKit Review Bot]

Created attachment 155361 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[David Grogan]

Ah yes, this won't run until shared.js from 92560 is in.

[Joshua Bell]

This happens because we'll try a delete, it's blocked, fire blocked event, queue a pending delete it, dequeue the pending delete, try again, it's still blocked, fire (second) blocked event?

[David Grogan]

(In reply to comment #6)
> This happens because we'll try a delete, it's blocked, fire blocked event, queue a pending delete it, dequeue the pending delete, try again, it's still blocked, fire (second) blocked event?

Correct.

[David Grogan]

(In reply to comment #6)
> This happens because we'll try a delete, it's blocked, fire blocked event, queue a pending delete it, dequeue the pending delete, try again, it's still blocked, fire (second) blocked event?

We could fiddle with the logic in IDBDatabaseBackendImpl, or we could take the easy route and make IDBRequest drop blocked events after the first.

[Joshua Bell]

I want to fiddle with the logic in IDBDatabaseBackendImpl. :)

Looking at the spec, for open() and deleteDatabase() the terminology used is "The method then queues up an operation to run the steps for XXXing a database." We implement this with the pending calls mechanism and multiple queues. We use the same methods for the initial call (open, deleteDatabase) and when we dequeue the events to try again, which is how we hit this.

One bit of cleanup might be to split those into deleteDatabase/deleteDatabaseInternal (like we do for put/putInternal in IDBObjectStore); in the spec, the steps for open/versionchange/delete are broken into two parts by a "wait until..." phrase. Parts before that should be in the (e.g.) deleteDatabase; the "wait until..." test could be in the processPendingCalls, and the rest in deleteDatabaseInternal.

[Joshua Bell]

Um, but waiting until "upgradeneeded" is DONE DONE DONE first, of course.

[Joshua Bell]

Playing with this a bit, the following pattern seems to work:

void deleteDatabase(callbacks); // steps before the "wait" predicate
bool isDeleteDatabaseBlocked(); // implements the "wait" predicate
void deleteDatabaseFinal(callbacks); // steps after the "wait" predicate

deleteDatabase(callbacks) adds a PendingDeleteDatabase object to the queue, then immediately calls processPendingCalls.

Repeat for open, openWithVersion, setVersion, etc. Note that openWithVersion may in turn spawn a setIntVersion, which itself needs to be split up.

The relevant chunk of processPendingCalls then becomes:

if (isDeleteDatabaseBlocked())
  return;
while (!m_pendingDeleteDatabaseCalls.isEmpty()) {
  PendingDeleteDatabase pending = m_pendingDeleteDatabaseCalls.takeFirst();
  deleteDatabaseFinal(pending.callbacks);
}

The version change bits (int and string) are the complicated ones, but it looks like this would simplify things overall.

[Joshua Bell]

Created attachment 158936 [details]
Patch

[Joshua Bell]

Whoops, didn't mean to overwrite the tests in Patch #2. 

This is just a work-in-progress to show what I was thinking. Untested and won't even compile.

[Joshua Bell]

Created attachment 160272 [details]
Patch

[Joshua Bell]

Another work-in-progress. Includes the tests (which pass!) from the first patches.

Runs, but a handful of tests differ in output due to the order in which various database lifecycle events are delivered. Unclear if this is desirable or not. This patch also aggressively renames many methods (runIntVersionChangeTransaction => setIntVersion, setVersionInternal => setVersionTask, resetVersion => resetVersionAbortTask, etc) just to retain my sanity.

We probably want to table this patch and just use it as a guide, doing the refactoring more slowly and maintaining test compatibility the whole time.

[Joshua Bell]

Created attachment 160475 [details]
Patch

[Joshua Bell]

Latest patch is a subset of the previous, built from scratch, plus the tests.

This just splits up deleteDatabase() into deleteDatabase()/isDeleteDatabaseBlocked()/deleteDatabaseFinal() and adjusts processPendingCalls() accordingly to resolve the bug. Some minor renaming is done (XXXInternal -> XXXTask), but leaving other cleanup for later.

dgrogan@ - please take a look

(We should check the flakiness dashboard before/after this lands.)

[David Grogan]

(In reply to comment #17)
> (We should check the flakiness dashboard before/after this lands.)

Anecdote: the first time I ran the layout tests with this patch key-generator.html timed out.  The second and third time it passed.

[WebKit Review Bot]

Comment on attachment 160475 [details]
Patch

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

New failing tests:
storage/indexeddb/factory-deletedatabase-interactions.html

[WebKit Review Bot]

Created attachment 160501 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Joshua Bell]

(In reply to comment #19)
> New failing tests:
> storage/indexeddb/factory-deletedatabase-interactions.html

That's interesting. Passes locally. So why is it now flaky?

[David Grogan]

Comment on attachment 160475 [details]
Patch

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

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions-expected.txt:46
> +'h.setVersion.onsuccess'

When run as a browser test, h.setVersion.onsuccess shows up right before 'h.onversionchange'.

[Joshua Bell]

(In reply to comment #22)
> (From update of attachment 160475 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160475&action=review
> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase-interactions-expected.txt:46
> > +'h.setVersion.onsuccess'
> 
> When run as a browser test, h.setVersion.onsuccess shows up right before 'h.onversionchange'.

Fun. With this patch:

In DRT the call order is:

IDBDatabase::setVersion
IDBDatabaseBackendImpl::setVersion
IDBFactory::deleteDatabase
IDBDatabaseBackendImpl::deleteDatabase
>> callbacks->onVersionChange()
IDBDatabaseBackendImpl::setVersionInternal (via task timer)
>> callbacks->onSuccess()

In Chromium the call order can instead be:

IDBDatabase::setVersion
IDBDatabaseBackendImpl::setVersion (via sync IPC)
IDBFactory::deleteDatabase
IDBDatabaseBackendImpl::setVersionInternal (via task timer)
>> callbacks->onSuccess()
IDBDatabaseBackendImpl::deleteDatabase (via async IPC)
>> callbacks->onVersionChange()

Without this patch, the deleteDatabase is silently delayed by a running/pending setVersion rather than having a blocked event.

One approach might be to have the success callback fired by the setVersion method rather than the setVersionInternal method.

[Joshua Bell]

> One approach might be to have the success callback fired by the setVersion method rather than the setVersionInternal method.

Bleah, can't do that since the transaction may be arbitrarily delayed by others on the same connection.

[Joshua Bell]

Created attachment 180044 [details]
Patch

[Joshua Bell]

dgrogan@ - can you take a look?

I made sure the tests pass in both test_shell and content_shell - there are difference in event logging in this sequence:

openReq = indexedDB.open("db");
openReq.onupgradeneeded = function() {
  db = openReq.result;
  delReq = indexedDB.deleteDatabase("db");
  db.onversionchange = function() { db.close(); };
  delReq.onsuccess = logSuccessEvent;
};
openReq.onerror = logErrorEvent;

In content_shell the openReq.onerror fires before delReq.onsuccess. In test_shell it's the other way around.

[David Grogan]

Comment on attachment 180044 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        are also now sent out immediately when a "versionchange" transaction is running, rather

Do you know why we ever entered that limbo state instead of sending out events immediately? I looked at an old spec from when setVersion was still around but that doesn't mention waiting for versionchange transactions either.

> LayoutTests/storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange-expected.txt:-16
> -versionChangeComplete = true

Curious, do you know what FF and IE do here?

> LayoutTests/storage/indexeddb/resources/intversion-delete-in-upgradeneeded-abort.js:6
> +description("Test that a deleteDatabase called while handling an upgradeneeded event is queued and fires its events at the right time");

I haven't diff'd them, but what's the difference between this and intversion-delete-in-upgradeneeded? Can you update this description?

> LayoutTests/storage/indexeddb/resources/intversion-delete-in-upgradeneeded.js:18
>  var sawFirstUpgradeNeeded = false;

It looks like there is no "secondUpgradeNeeded", could you change this to sawUpgradeNeeded while you're in here?

[Joshua Bell]

Comment on attachment 180044 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        are also now sent out immediately when a "versionchange" transaction is running, rather
> 
> Do you know why we ever entered that limbo state instead of sending out events immediately? I looked at an old spec from when setVersion was still around but that doesn't mention waiting for versionchange transactions either.

My guess is that the deleteDatabase implementation cribbed from setVersion, and with that API you have to support having the same connection call setVersion() twice, and in that case you probably don't want events.

>> LayoutTests/storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange-expected.txt:-16
>> -versionChangeComplete = true
> 
> Curious, do you know what FF and IE do here?

Ah, very good question. (*runs off and tries it*)

IE10:
* call open()
* open request's onupgradeneeded fires
* call deleteDatabase()
* delete request's onblocked fires
* versionchange transaction completes
* open request's onsuccess fires
* call close()
* delete request's onsuccess fires

FF17/19:
* call open()
* open request's onupgradeneeded fires
* call deleteDatabase()
* versionchange transaction completes
* open request's onsuccess fires
* call close()
* delete request's onsuccess fires

Confirmed w/ jonas on IRC that not firing "blocked" here is a FF bug. With this patch we match IE.

>> LayoutTests/storage/indexeddb/resources/intversion-delete-in-upgradeneeded-abort.js:6
>> +description("Test that a deleteDatabase called while handling an upgradeneeded event is queued and fires its events at the right time");
> 
> I haven't diff'd them, but what's the difference between this and intversion-delete-in-upgradeneeded? Can you update this description?

One calls close() within onversionchange, one calls close() in open-onsuccess. The former should abort the open, so I called that one "-abort".

Calling close() in onversionchange is what this test originally did, but since onversionchange fires earlier it altered the flow of the test. So I moved close() to open-onsuccess, and added a separate test to capture calling close in onversionchange.

Will update the desc and possibly rename the test(s).

>> LayoutTests/storage/indexeddb/resources/intversion-delete-in-upgradeneeded.js:18
>>  var sawFirstUpgradeNeeded = false;
> 
> It looks like there is no "secondUpgradeNeeded", could you change this to sawUpgradeNeeded while you're in here?

Will do.

[Joshua Bell]

Created attachment 180236 [details]
Patch

[Joshua Bell]

tony@ - can you review?

[WebKit Review Bot]

Comment on attachment 180236 [details]
Patch

Clearing flags on attachment: 180236

Committed r138268: <http://trac.webkit.org/changeset/138268>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.