Closed Bug 608186 Opened 9 years ago Closed 9 years ago

IndexedDB: Transactions should expire when we return to the event loop

Categories

(Core :: DOM: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Details

Attachments

(2 files)

Attached patch Patch, v1Splinter Review
The following code is currently racy:

  var transaction = db.transaction("foo", IDBTransaction.READ_WRITE);
  transaction.objectStore("foo").add({}).onsuccess = function(event) {
    ...
  };
  setTimeout(function() { transaction.objectStore("foo").add({}); }, 0);

If the setTimeout executes before the success callback is fired then the second add call will succeed as well as the first. If the success callback runs before the timeout then the second add will fail. This needs to be fixed.

The proposed solution (discussed and agreed upon by the spec authors) is to only allow requests to be made against a transaction in two cases:

1. The transaction has just been created. Requests can be made until the function returns and we process another event in the event loop.
2. We're in the transaction's success callback. Requests can be made until the function returns.

The attached patch installs a thread observer to monitor the event loop and notify newly created transactions.
Attachment #486840 - Flags: review?(jonas)
Comment on attachment 486840 [details] [diff] [review]
Patch, v1

>+AsyncConnectionHelper::SetCurrentTransaction(IDBTransaction* aTransaction)
>+{
>+  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>+
>+  if (aTransaction) {
>+    NS_ASSERTION(!gCurrentTransaction, "Overwriting current transaction!");
>+  }

Why not assert both ways here? I.e. something like

NS_ASSERTION(!gCurrentTransaction != !aTransaction, "...");

> IDBTransaction::Create(IDBDatabase* aDatabase,
...
>+  if (!ThreadObserver::BeginObserving(transaction)) {
>+    return nsnull;
>+  }
>+
>+  transaction->mCreating = true;

Instead of doing this, when aDelayedDispatch is true don't observe for this transaction and set mCreating to false. You might not even need the mDelayedDispatch member.

>+ThreadObserver::UpdateNewlyCreatedTransactions()
>+{
>+  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>+
>+  if (!mTransactions.IsEmpty()) {

Remove this if-check, it doesn't save you many, if any, cycles.

>+ThreadObserver::AfterProcessNextEvent(nsIThreadInternal* aThread,

Call UpdateNewlyCreatedTransactions from here rather than from the dtor.

Also need to fix the recursion-depth and the there-might-be-others-inserting-themselves-in-the-chain thing as discussed.
Attachment #486840 - Flags: review?(jonas) → review-
We need this for beta8.
blocking2.0: betaN+ → beta8+
Attached patch FixesSplinter Review
Here's an interdiff of changes. What do you think?
Attachment #489968 - Flags: review?(jonas)
Comment on attachment 489968 [details] [diff] [review]
Fixes

You didn't make the change to SetCurrentTransaction as requested in comment 1. r=me either way though.
Attachment #489968 - Flags: review?(jonas) → review+
http://hg.mozilla.org/mozilla-central/rev/d62eda4e4137

I added another test too.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.