Bug 89379 - IndexedDB: Implement IDBTransaction internal active flag
Summary: IndexedDB: Implement IDBTransaction internal active flag
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
Depends on: 90089
  Show dependency treegraph
Reported: 2012-06-18 14:09 PDT by Joshua Bell
Modified: 2012-06-28 17:38 PDT (History)
9 users (show)

See Also:

Patch (86.26 KB, patch)
2012-06-26 17:40 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (84.93 KB, patch)
2012-06-27 14:29 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (84.88 KB, patch)
2012-06-28 14:22 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-06-18 14:09:43 PDT
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."

"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.
Comment 1 Joshua Bell 2012-06-18 14:30:15 PDT
A plausible test for this looks like:

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

function doRequest() {
  if (done)
  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
Comment 2 Joshua Bell 2012-06-18 15:27:14 PDT
Looks like I'll have to do this as part of webkit.org/b/89377
Comment 3 Joshua Bell 2012-06-19 09:38:40 PDT
Also, don't abort empty transactions - commit them instead. (May require plumbing for Chromium port.)
Comment 4 Joshua Bell 2012-06-26 17:40:55 PDT
Created attachment 149646 [details]
Comment 5 Joshua Bell 2012-06-26 17:43:57 PDT
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.
Comment 6 Alec Flett 2012-06-27 11:56:02 PDT
Comment on attachment 149646 [details]

This all looks quite reasonable, I wish I had more to add but it seems fine.
Comment 7 Joshua Bell 2012-06-27 11:57:49 PDT
Chromium WebKit public API addition is at:

Comment 8 Joshua Bell 2012-06-27 13:09:10 PDT
tony@ - can you take a look at your leisure? Various other things need to land before this can be submitted.
Comment 9 Tony Chang 2012-06-27 14:05:35 PDT
Comment on attachment 149646 [details]

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> > ?
Comment 10 Joshua Bell 2012-06-27 14:29:39 PDT
Created attachment 149795 [details]
Comment 11 Joshua Bell 2012-06-27 14:31:44 PDT
(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.
Comment 12 Joshua Bell 2012-06-28 14:22:11 PDT
Created attachment 150014 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-06-28 17:38:25 PDT
Comment on attachment 150014 [details]
Patch for landing

Clearing flags on attachment: 150014

Committed r121492: <http://trac.webkit.org/changeset/121492>
Comment 14 WebKit Review Bot 2012-06-28 17:38:30 PDT
All reviewed patches have been landed.  Closing bug.