1149815 - Lots of asserts/crashes running wpt IndexedDB tests in debug builds

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[James Graham [:jgraham]]

When running the web-platform-tests in debug builds I see a lot of asserts and crashes in IndexedDB tests e.g. [1]. Note that the orange jobs there are just the ones with *unexpected* (i.e. intermittent) crashes, the other jobs have similar, but more stable, crashes that have been marked as "expected". 

[1] https://treeherder.mozilla.org/#/jobs?repo=cedar&revision=3116e1bb2004&filter-searchStr=web-platform-tests-1%20debug

Comment 1

[James Graham [:jgraham]]

The assertion that's failing here seems to be "Assertion failure: !metadata->mDeleted"

Comment 2

[James Graham [:jgraham]]

At a few different line numbers:


PROCESS | 1824 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12238
PROCESS-CRASH | /IndexedDB/idbcursor_delete_index4.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForObjectStoreId]
PROCESS | 2747 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12238
PROCESS-CRASH | /IndexedDB/idbcursor_delete_objectstore4.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForObjectStoreId]
PROCESS | 2814 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12238
PROCESS-CRASH | /IndexedDB/idbcursor_update_index4.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForObjectStoreId]
PROCESS | 2886 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12238
PROCESS-CRASH | /IndexedDB/idbcursor_update_objectstore5.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForObjectStoreId]
PROCESS | 2955 | Assertion failure: mState == State_WaitingForOtherDatabasesToClose, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:17710
PROCESS-CRASH | /IndexedDB/idbfactory_open12.htm | application crashed [@ mozilla::dom::indexedDB::::OpenDatabaseOp::NoteDatabaseClosed]

    1112716 Intermittent test_blocked_order.html | application crashed [@ mozilla::dom::workers::WorkerThread::Dispatch(nsIRunnable*, unsigned int)][@ mozilla::dom::indexedDB::::OpenDatabaseOp::NoteDatabaseClosed]

PROCESS | 3066 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12261
PROCESS-CRASH | /IndexedDB/idbindex_get6.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForIndexId]
PROCESS | 3148 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12261
PROCESS-CRASH | /IndexedDB/idbindex_getKey6.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForIndexId]
PROCESS | 3215 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12261
PROCESS-CRASH | /IndexedDB/idbindex_openCursor.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForIndexId]
PROCESS | 3278 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12261
PROCESS-CRASH | /IndexedDB/idbindex_openKeyCursor2.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForIndexId]
PROCESS | 3341 | Assertion failure: !metadata->mDeleted, at /builds/slave/ced-l64-d-00000000000000000000/build/src/dom/indexedDB/ActorsParent.cpp:12238
PROCESS-CRASH | /IndexedDB/idbobjectstore_deleted.htm | application crashed [@ mozilla::dom::indexedDB::::TransactionBase::GetMetadataForObjectStoreId]

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Any objection to removing those assertions?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to :Ms2ger from comment #3)
> Any objection to removing those assertions?

Yes, they're there for a reason. I'll try to catch in rr.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This doesn't look like a race at all, we're just not properly guarding against deleted objectstores/indexes in some cases.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 1: Pass storage function failure codes through to callers, v1

Storage is eating the result codes from custom functions and always returning a generic SQLITE_ERROR. This fix allows us to pass failure codes through properly.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 2: Guard against using deleted IDBObjectStore and IDBIndex objects, v1

We're not doing a great job of preventing JS from using deleted IDBObjectStore and IDBIndex objects here, and apparently we don't have our own tests for that.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 3: Don't assume that index creation always succeeds, v1

I added an optimization to avoid modifying index data if we knew that the objectstore didn't have indexes. The optimization will be defeated (and an assertion will fire) if we're in a versionchange transaction and we try to create an index but it fails. This patch fixes the optimization.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8623181 [details] [diff] [review]
Part 1: Pass storage function failure codes through to callers, v1

Review of attachment 8623181 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good and correct.  sqlite3_result_error is correctly called before sqlite3_result_error_code to avoid value clobbering and the string handling properly gets the whole nul-terminated string.

Comment 10

[James Graham [:jgraham]]

Early results are promising, but I'm seeing a few unexpected fails and at least one assert in debug: https://treeherder.mozilla.org/#/jobs?repo=cedar&revision=0325e7cdfb17&filter-searchStr=web-platform

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 4: Properly handle IDBDatabase.close() called during a versionchange transaction, v1

The spec says that we should let the transaction commit but still fire an error event with AbortErr on the IDBOpenDBRequest object. This change causes a few xpcshell failures so those changes are included here.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 5: Fix IndexedDB tests to expect InvalidStateError rather than TransactionInactiveError when touching a deleted IDBObjectStore or IDBIndex object

Some of the tests were expecting the wrong error code.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 6: Enable wpt tests that now pass.

And we should run these tests now that they pass.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ee1f819908

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #14)

A few mochitests need to be tightened up too it seems.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Part 4: Properly handle IDBDatabase.close() called during a versionchange transaction, v1

Additional test changes only, now try is happy.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53d624e8a021

Comment 18

[Jan Varga [:janv]]

Comment on attachment 8624594 [details] [diff] [review]
Part 4: Properly handle IDBDatabase.close() called during a versionchange transaction, v1

Review of attachment 8624594 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6df9bf6c539
https://hg.mozilla.org/integration/mozilla-inbound/rev/826980ade3b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f270dc49a348
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e5be3e74b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/080de2337bb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a1630c8660

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6df9bf6c539
https://hg.mozilla.org/mozilla-central/rev/826980ade3b2
https://hg.mozilla.org/mozilla-central/rev/f270dc49a348
https://hg.mozilla.org/mozilla-central/rev/a9e5be3e74b2
https://hg.mozilla.org/mozilla-central/rev/080de2337bb9
https://hg.mozilla.org/mozilla-central/rev/81a1630c8660

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8623410 [details] [diff] [review]
Part 6: Enable wpt tests that now pass.

Approval Request Comment
[Feature/regressing bug #]: This non-spec-compliant behavior is very old, probably since the first version if IndexedDB. 
[User impact if declined]: Broken behavior in several edge cases
[Describe test coverage new/current, TreeHerder]: WPT tests
[Risks and why]: Low risk, just changes behavior to be spec compliant in edge cases.
[String/UUID change made/needed]: None

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Ben, I am not sure to understand why this cannot ride the trains. It is pretty big patches and I am not sure to see the gains for users.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

They can, I just don't think the patches are very risky and we could fix broken behavior sooner.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

OK. So, let's it ride the train. We have this issue for a while, no need to ask risk to the 40 release then.