It's possible to run CommitOp without ever having really run StartTransactionOp

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed)

Details

Attachments

(1 attachment)

Because StartTransactionOp is a TransactionDatabaseOperationBase, while CommitOp is only a DatabaseOperationBase, if the database is invalidated before StartTransactionOp executes on the connection thread it will not execute DoDatabaseWork, and then when CommitOp runs if we had a write transaction it will explode because we never entered the write transaction.

This is responsible for the various !mInReadTransaction assertions.

Rather than reworking one of them to inherit from the same base class it seems easier just to track the transaction started state on the connection thread too, and not execute the CommitOp if we didn't start on the connection thread.
Created attachment 8634517 [details] [diff] [review]
Patch

I'm fairly confident that it is impossible to test this reliably.
Attachment #8634517 - Flags: review?(Jan.Varga)

Comment 2

4 years ago
Comment on attachment 8634517 [details] [diff] [review]
Patch

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

Ok
Attachment #8634517 - Flags: review?(Jan.Varga) → review+
https://hg.mozilla.org/mozilla-central/rev/7e57d7f2d2c0
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8634517 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: IndexedDB implementation
[User impact if declined]: Intermittent failures of tests or of web pages if tabs are closed or navigated at exactly the wrong time.
[Describe test coverage new/current, TreeHerder]: Covered by automated tests.
[Risks and why]: Low risk
[String/UUID change made/needed]: N/A
Attachment #8634517 - Flags: approval-mozilla-aurora?
Comment on attachment 8634517 [details] [diff] [review]
Patch

Has been in m-c for a few days without any known negative impact. This patch also has coverage from automated tests so should be safe to uplift to Aurora.
Attachment #8634517 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox41: --- → affected
You need to log in before you can comment on or make changes to this bug.