Bug 77060 - IndexedDB: Key generators not rolled back if insertion fails or is aborted
Summary: IndexedDB: Key generators not rolled back if insertion fails or is aborted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on: 76952
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-25 16:53 PST by Joshua Bell
Modified: 2012-02-03 16:17 PST (History)
4 users (show)

See Also:


Attachments
Layout Test validating key generator cases (7.43 KB, text/html)
2012-01-27 12:16 PST, Joshua Bell
no flags Details
Patch (19.01 KB, patch)
2012-01-31 16:25 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (18.97 KB, patch)
2012-02-03 10:13 PST, 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-01-25 16:53:04 PST
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.
Comment 1 Joshua Bell 2012-01-27 12:16:44 PST
Created attachment 124347 [details]
Layout Test validating key generator cases
Comment 2 Joshua Bell 2012-01-30 15:23:53 PST
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.)
Comment 3 Joshua Bell 2012-01-30 15:26:01 PST
Don't try and fix this until the refactoring in https://bugs.webkit.org/show_bug.cgi?id=76952 lands
Comment 4 Joshua Bell 2012-01-31 16:25:46 PST
Created attachment 124843 [details]
Patch
Comment 5 Joshua Bell 2012-01-31 16:30:37 PST
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.
Comment 6 David Grogan 2012-02-02 19:13:28 PST
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
Comment 7 Joshua Bell 2012-02-03 10:13:02 PST
Created attachment 125359 [details]
Patch
Comment 8 Joshua Bell 2012-02-03 10:21:18 PST
(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?
Comment 9 Joshua Bell 2012-02-03 13:00:42 PST
After offline discussion with dgrogan@, we think this patch is acceptable and probably the best approach, short of a major refactor.
Comment 10 Joshua Bell 2012-02-03 14:56:22 PST
tony@, can you take a look?
Comment 11 WebKit Review Bot 2012-02-03 16:17:01 PST
Comment on attachment 125359 [details]
Patch

Clearing flags on attachment: 125359

Committed r106705: <http://trac.webkit.org/changeset/106705>
Comment 12 WebKit Review Bot 2012-02-03 16:17:06 PST
All reviewed patches have been landed.  Closing bug.