IndexedDB: Avoid crashing when deleting indexes

[Alec Flett]

IndexedDB: Avoid crashing when deleting indexes

[Alec Flett]

Created attachment 185519 [details]
Patch

[Alec Flett]

jsbell@ - I hit a crash when debugging something else, so I decided to unearth this patch.

We talked about this before, but basically we're just allowing transactionIds to be invalid.

[Joshua Bell]

Comment on attachment 185519 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:560
> +    if (!m_transactions.contains(transactionId))

Do you think we should add comments? They'd be in a lot of places... possibly just in the first instance (since it's all in one file now) explain that this just means the transaction has aborted asynchronously from this incoming request.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:563
>      IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);

Add ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE); since it's in deleteObjectStore etc?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:605
> +    IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);

Here and elsewhere, to avoid the contains/get, can just be:

IDBTransactionBackendImpl* transaction = m_transactions.get(transactionId);
if (!transaction)
  return;

> LayoutTests/ChangeLog:10
> +        with this patch but was the test condition that uncovered the overall problem

s/with/without/ ?

Is the plan to run this test on the Chromium side as well to watch for flaky crashes == regressions? Were you able to get the failure to repro in content_shell?

> LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js:9
> + {

Extra whitespace?

> LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js:10
> +  var date = new Date();

Should be indented 4 spaces.

Also, can use Date.now() to get the current time in millis w/o creating a temp object. So can be just:

var target = Date.now() + millis;
while (Date.now() < target) {
    // busy wait
}

Aside: I wonder if there's a sleep() API in Internals?

> LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js:12
> +  do { curDate = new Date(); }

Put the do...while either all on one line or three lines. do { \n ... \n } while (...);

> LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js:22
> +    db.createObjectStore("objectStore");

Are these lines intentionally not using evalAndLog() for some reason?

> LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js:31
> +    // this will asynchronously abort in the backend because of constraint failures

Capitalize/punctuation.

> LayoutTests/storage/indexeddb/resources/createIndex-after-failure.js:33
> +    // now immediately delete it.

Ditto, etc.

> LayoutTests/storage/indexeddb/resources/shared.js:208
> +function waitForRequests(requests, callback) {

The { of a top-level function should be on its own line. (This is style guideline is violated in some of the preceding functions.)

> LayoutTests/storage/indexeddb/resources/shared.js:211
> +    if (count == 0) callback(requests);

Return early here (just for readability).

> LayoutTests/storage/indexeddb/resources/shared.js:213
> +    requests.forEach(function(req) {

Technically, IDB requests from the same transaction must serviced in the order they're issued, so this could skip the closure-over-count and just set onsuccess on the last request. But I like the Promises-like generalization. :)

[Alec Flett]

Created attachment 185525 [details]
Patch

[Alec Flett]

tony@ - r?

[Tony Chang]

Comment on attachment 185525 [details]
Patch

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

> LayoutTests/storage/indexeddb/resources/shared.js:211
> +    if (count == 0) {

Nit: !count

> LayoutTests/storage/indexeddb/resources/shared.js:218
> +            count--;

Nit: --count.

> LayoutTests/storage/indexeddb/resources/shared.js:219
> +            if (count == 0)

Nit: !count

[Alec Flett]

Created attachment 185530 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 185530 [details]
Patch for landing

Rejecting attachment 185530 [details] from commit-queue.

New failing tests:
storage/indexeddb/createIndex-after-failure.html
Full output: http://queues.webkit.org/results/16115908

[Alec Flett]

Created attachment 185554 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 185554 [details]
Patch for landing

Clearing flags on attachment: 185554

Committed r141316: <http://trac.webkit.org/changeset/141316>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.