Bug 80547 – IndexedDB: Creating a transaction from within a transaction callback should fail

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

IndexedDB: Creating a transaction from within a transaction callback should fail

[Joshua Bell]

Created attachment 130709 [details]
Patch

[Joshua Bell]

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.

[Joshua Bell]

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.

[David Grogan]

LGTM

(I looked through this last night but forgot to LGTM it.)

[Joshua Bell]

tony@, r?

Opinions on landing this now? I'm still torn between inflicting pain now vs. later.

[Joshua Bell]

Created attachment 132339 [details]
Patch

[Joshua Bell]

Comment on attachment 132339 [details]
Patch

FYI, latest patch is just a rebase/merge against the recent test refactoring.

[Tony Chang]

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.

[Joshua Bell]

Created attachment 133135 [details]
Patch

[Joshua Bell]

Comment on attachment 133135 [details]
Patch

Just a rebase on top of r111225 which took care of the test updated.

[Joshua Bell]

Created attachment 135709 [details]
Patch

[David Grogan]

Comment on attachment 135709 [details]
Patch

LGTM

[Joshua Bell]

Handing this off to dgrogan@ - let's land it at the same time as onupgradeneeded.

[Joshua Bell]

Alec's going to take ownership of this w/ the error cleanup he's doing, and probably slip this in.

[Alec Flett]

Created attachment 147390 [details]
Patch

[Alec Flett]

jsbell@ / dgrogan@ - mind taking a look?

[Joshua Bell]

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

[Alec Flett]

Created attachment 147435 [details]
Patch

[Alec Flett]

Comment on attachment 147435 [details]
Patch

jsbell's issues addressed.

tony@ - r? cq?

[Alec Flett]

Created attachment 147436 [details]
Patch

[Alec Flett]

Comment on attachment 147436 [details]
Patch

ooops, forgot to update expectations.

new patch, tony@ - r? cq?

[WebKit Review Bot]

Comment on attachment 147436 [details]
Patch

Clearing flags on attachment: 147436

Committed r120295: <http://trac.webkit.org/changeset/120295>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.