Bug 77060 – IndexedDB: Key generators not rolled back if insertion fails or is aborted

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

See thread: http://lists.w3.org/Archives/Public/public-webapps/2012JanMar/0329.html

Chromium port of WebKit behaves differently in these cases:


If an insertion fails due to constraint violations or IO error, the
key generator is not updated.
trans.onerror = function(e) { e.preventDefault() };
store = db.createObjectStore("store1", { autoIncrement: true });
index = store.createIndex("index1", "ix", { unique: true });
store.put({ ix: "a"}); // Will get key 1
store.put({ ix: "a"}); // Will fail
store.put({ ix: "b"}); // Will get key 2  -- Chromium/WebKit gets 3

....

Aborting a transaction rolls back any increases to the key generator
which happened during the transaction. This is to make all rollbacks
consistent since rollbacks that happen due to crash never has a chance
to commit the increased key generator value.
db.createObjectStore("store", { autoIncrement: true });
...
trans1 = db.transaction(["store"]);
store_t1 = trans1.objectStore("store");
store_t1.put("a"); // Will get key 1
store_t1.put("b"); // Will get key 2
trans1.abort();
trans2 = db.transaction(["store"]);
store_t2 = trans2.objectStore("store");
store_t2.put("c"); // Will get key 1 -- Chromium/WebKit gets 3
store_t2.put("d"); // Will get key 2 -- Chromium/WebKit gets 4

.....

In the former case, it appears that the failed put does not roll back the key generator. The uniqueness test should run before the key generator is used.

In the latter case, it appears that the aborted transaction does not roll back the key generator. The key generator state should be included in the transaction scope.

[Joshua Bell]

Created attachment 124347 [details]
Layout Test validating key generator cases

[Joshua Bell]

From a preliminary glance, it looks like the fix should just require a call to IDBObjectStoreBackendImpl::resetAutoIncrementKeyCache() in the failure cases within IDBObjectStoreBackendImpl::putInternal()

(The sad news is that our key generator "state" is pretty sucktastic - resetAutoIncrementKeyCache() just invalidates a cache. When the cache is invalidated, we populate it with a table scan. Yuck. But it works.)

[Joshua Bell]

Don't try and fix this until the refactoring in https://bugs.webkit.org/show_bug.cgi?id=76952 lands

[Joshua Bell]

Created attachment 124843 [details]
Patch

[Joshua Bell]

I'm not terribly happy with this patch, but it works.

Rather than maintain the key generator state as part of the object store (per spec) we infer the "next key" by doing a table scan. Ugh. The object store implementation simply caches this and will invalidate the cache if something is inserted that would modify the key generator.

The patch simply invalidates if a put() fails or if the transaction aborts. The former requires sprinkling lots of calls throughout the put task. The latter uses abort tasks, which are redundant since they just invalidate the state.

A better fix is (1) the key generator state is held in the backing store as a property of the object store so it can be rolled back with transactions, and (2) store objects are scoped within transactions so they don't even need abort tasks. But that's a huge architectural change.

[David Grogan]

Comment on attachment 124843 [details]
Patch

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

LGTM

> LayoutTests/storage/indexeddb/key-generator.html:151
> +    'Verify that the key generator is only set if and only if a numeric key greater than the last generatd key is used.',

typo generatd

> LayoutTests/storage/indexeddb/key-generator.html:204
> +                console.log('aborted!');

Are these console.log calls intentional?

> LayoutTests/storage/indexeddb/key-generator.html:209
> +                check(store_t2, 1, 'c'); // FIXME: Fails in Chromium (it gets key 3).

Remove these FIXMEs

[Joshua Bell]

Created attachment 125359 [details]
Patch

[Joshua Bell]

(In reply to comment #6)
> (From update of attachment 124843 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124843&action=review
> 
> LGTM
> 
> > LayoutTests/storage/indexeddb/key-generator.html:151
> > +    'Verify that the key generator is only set if and only if a numeric key greater than the last generatd key is used.',
> 
> typo generatd

Fixed.

> > LayoutTests/storage/indexeddb/key-generator.html:204
> > +                console.log('aborted!');
> 
> Are these console.log calls intentional?

Oops, no. Changed them to debug() calls.

> > LayoutTests/storage/indexeddb/key-generator.html:209
> > +                check(store_t2, 1, 'c'); // FIXME: Fails in Chromium (it gets key 3).
> 
> Remove these FIXMEs

Done.

I still want to brainstorm further before landing this. 

* A safer alternative to sprinkling the reset...() calls into putInternal()'s early exits would be to introduce a class that does RAII-style revert on early exit. 

* A more performant/less memory hungry alternative to the per-put abort task would be to have the IDBTransactionBackendImpl call into a new "transactionAborted" method on each IDBObjectStoreBackendImpl.

Thoughts?

[Joshua Bell]

After offline discussion with dgrogan@, we think this patch is acceptable and probably the best approach, short of a major refactor.

[Joshua Bell]

tony@, can you take a look?

[WebKit Review Bot]

Comment on attachment 125359 [details]
Patch

Clearing flags on attachment: 125359

Committed r106705: <http://trac.webkit.org/changeset/106705>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.