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.
Created attachment 124347 [details] Layout Test validating key generator cases
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.)
Don't try and fix this until the refactoring in https://bugs.webkit.org/show_bug.cgi?id=76952 lands
Created attachment 124843 [details] Patch
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 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
Created attachment 125359 [details] Patch
(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?
After offline discussion with dgrogan@, we think this patch is acceptable and probably the best approach, short of a major refactor.
tony@, can you take a look?
Comment on attachment 125359 [details] Patch Clearing flags on attachment: 125359 Committed r106705: <http://trac.webkit.org/changeset/106705>
All reviewed patches have been landed. Closing bug.