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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Attachment: Patch, v1

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Johnny Stenback (:jst)]

We need this for beta8.

Comment 3

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

Attachment: Fixes

Here's an interdiff of changes. What do you think?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

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

http://hg.mozilla.org/mozilla-central/rev/d62eda4e4137

I added another test too.