Bug 89379 – IndexedDB: Implement IDBTransaction internal active flag

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

The IDB spec http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html says:

Transaction lifecycle:
"Transactions have an active flag, which determines if new requests can be made against the transaction."
"When a transaction is created its active flag is initially set to true." - i.e. right after the db.transaction() call
"The implementation must allow requests to be placed against the transaction whenever the active flag is true."
"Requests may be placed against a transaction only while that transaction is active. If an attempt is made to place a request against a transaction when that transaction is not active, the implementation must reject the attempt by throwing a DOMException of type TransactionInactiveError."

Version changes:
"This transaction will be active inside the onupgradeneeded event handler, allowing the creation of new object stores and indexes."

Abort:
"Otherwise this method sets the transaction's active flag to false..."

Transaction creation:
"When control is returned to the event loop, the implementation must set the active flag to false."

Firing events:
...
"Set the active flag of transaction to true."
...
"Set the active flag of transaction to false."
...

Right now we don't have a notion of non-active transaction - until it is |finished| we allow requests to be placed against it.

[Joshua Bell]

A plausible test for this looks like:

trans = db.transaction('store');
trans.oncomplete = finishJSTest;
var done = false;

function doRequest() {
  if (done)
    return;
  trans.objectStore('store').get('key').onsuccess = doRequest;
};

setTimeout(function () {
    evalAndExpectException("trans.objectStore('store').get('key')", "IDBDatabaseException.TRANSACTION_INACTIVE_ERR", "'TransactionInactiveError'");
    done = true;
}, 0);

In other words, show

[Joshua Bell]

Looks like I'll have to do this as part of webkit.org/b/89377

[Joshua Bell]

Also, don't abort empty transactions - commit them instead. (May require plumbing for Chromium port.)

[Joshua Bell]

Created attachment 149646 [details]
Patch

[Joshua Bell]

alecflett@ - please take a look

Landing sequence will be:
(1) Trivial patch with WebKit::WebIDBTransaction::commit() 
(2) Chromium patch for plumbing through commit() and disabling some layout-tests-as-browser-tests that are getting renamed
(3) This patch
(4) Re-enable the layout-tests-as-browser-tests

Forgot to mention in the ChangeLog, but the transaction-complete-XXX tests are renames of the transaction-abort-XXX tests, now that empty transactions commit rather than abort.

[Alec Flett]

Comment on attachment 149646 [details]
Patch

This all looks quite reasonable, I wish I had more to add but it seems fine.

[Joshua Bell]

Chromium WebKit public API addition is at:

http://webkit.org/b/90089

[Joshua Bell]

tony@ - can you take a look at your leisure? Various other things need to land before this can be submitted.

[Tony Chang]

Comment on attachment 149646 [details]
Patch

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

I don't understand all these changes, but it seems OK.

> Source/WebCore/Modules/indexeddb/IDBPendingTransactionMonitor.cpp:37
> +static ThreadSpecific<Vector<RefPtr<IDBTransaction> > >& transactions()

Nit: Maybe use a typedef for Vector<RefPtr<IDBTransaction> > ?

[Joshua Bell]

Created attachment 149795 [details]
Patch

[Joshua Bell]

(In reply to comment #9)
> 
> Nit: Maybe use a typedef for Vector<RefPtr<IDBTransaction> > ?

Good idea - done.

New patch just adds this and rebases on http://trac.webkit.org/changeset/121366

Waiting on that to roll into Chromium and landing http://codereview.chromium.org/10692017/ before this can land.

[Joshua Bell]

Created attachment 150014 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 150014 [details]
Patch for landing

Clearing flags on attachment: 150014

Committed r121492: <http://trac.webkit.org/changeset/121492>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.