IndexedDB: Creating a transaction from within a transaction callback should fail
Created attachment 130709 [details] Patch
Given that this is likely to break user code, I'm not sure we should land this before the onupgradeneeded change. That said, we would like to support both setVersion() and onupgradeneeded, so maybe it's okay to land this first, with sufficient warning on the mailing lists. And *that* said, I'm not entirely sure this is correct. As written, the patch only applies to VERSION_CHANGE transactions, i.e. those created in response to setVersion() calls. It precludes calling IDBDatabase.transaction as long as the VERSION_CHANGE transaction is active on the IDBDatabase, regardless of whether it's within the IDBVersionChangeRequest's callback or a dependent IDBRequest's callback. The text in https://www.w3.org/Bugs/Public/show_bug.cgi?id=11401 only applies to the Sync API. I'll follow up on the mailing list.
Ah, the spec now says re: IDBDatabase.transaction() "This method must throw a DOMException of type InvalidStateError if called before the success event for a open call has been dispatched." That applies to the newfangled approach: req = indexedDB.open(name, version); req.onupgradeneeded = function () { var db = req.result; // better not call db.transaction here var putreq = db.createObjectStore(...).put(...); putreq.onsuccess = function () { // better not call db.transaction here }; }; req.onsuccess = function () { var db = req.result; // you can finally call db.transaction now }; The upshot being that you can't call db.transaction while a VERSION_CHANGE transaction is running. This matches the patch's behavior change in spirit, even though the API is radically different. So I'm okay with landing this, now or later.
LGTM (I looked through this last night but forgot to LGTM it.)
tony@, r? Opinions on landing this now? I'm still torn between inflicting pain now vs. later.
Created attachment 132339 [details] Patch
Comment on attachment 132339 [details] Patch FYI, latest patch is just a rebase/merge against the recent test refactoring.
Comment on attachment 132339 [details] Patch The code looks fine. I defer to your judgement about whether to land now or after onupgradeneeded is supported. If there aren't that many clients, it might be OK to land this now.
Created attachment 133135 [details] Patch
Comment on attachment 133135 [details] Patch Just a rebase on top of r111225 which took care of the test updated.
Created attachment 135709 [details] Patch
Comment on attachment 135709 [details] Patch LGTM
Handing this off to dgrogan@ - let's land it at the same time as onupgradeneeded.
Alec's going to take ownership of this w/ the error cleanup he's doing, and probably slip this in.
Created attachment 147390 [details] Patch
jsbell@ / dgrogan@ - mind taking a look?
Comment on attachment 147390 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147390&action=review overall lgtm > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:86 > + , m_mode(mode) Add ASSERT in constructor that m_mode == m_backend->mode() ? > LayoutTests/storage/indexeddb/mozilla/resources/remove-objectstore.js:-47 > - if (++addedCount == 100) { Remove other uses of addedCount in this file (and -expected.txt) > LayoutTests/storage/indexeddb/resources/database-basics.js:32 > + Nit: remove blank line
Created attachment 147435 [details] Patch
Comment on attachment 147435 [details] Patch jsbell's issues addressed. tony@ - r? cq?
Created attachment 147436 [details] Patch
Comment on attachment 147436 [details] Patch ooops, forgot to update expectations. new patch, tony@ - r? cq?
Comment on attachment 147436 [details] Patch Clearing flags on attachment: 147436 Committed r120295: <http://trac.webkit.org/changeset/120295>
All reviewed patches have been landed. Closing bug.