Bug 91456 – IndexedDB: Key generator state not maintained across connections

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

Created attachment 152658 [details]
Repro case - should show different keys

The IDB spec http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#key-generator-concept says:

"The current number for a key generator never decreases, other than as a result of database operations being reverted. Deleting a record from an object store never affects the object store's key generator. Even clearing all records from an object store, for example using the clear() function, does not affect the current number of the object store's key generator."

The WebKit implementation violates this in this case:

db.open(...);
store.put(value1);
store.clear();
db.close();
db.open();
store.put(value2);

The same key will be used for value2 as was used for value1. This is because the key generator state is not actually saved, but derived from the maximum key in the database at the time it is first needed.

[Joshua Bell]

I'll take this. Should be pretty easy...

[Joshua Bell]

Created attachment 152806 [details]
Patch

[Joshua Bell]

dgrogan@, alecflett@ - please take a look.

Opinions on deferring updated caching logic to a subsequent patch?

[David Grogan]

(In reply to comment #3)
> dgrogan@, alecflett@ - please take a look.
> 
> Opinions on deferring updated caching logic to a subsequent patch?

Defer.

[WebKit Review Bot]

Comment on attachment 152806 [details]
Patch

Attachment 152806 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13279234

[Joshua Bell]

(In reply to comment #5)
> (From update of attachment 152806 [details])
> Attachment 152806 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13279234

Whoops, guess I didn't test a during-upload refactor. New patching coming momentarily...

[Joshua Bell]

Created attachment 152810 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152810 [details]
Patch

Attachment 152810 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13270241

[Joshua Bell]

Created attachment 152815 [details]
Patch

[Joshua Bell]

There we go - final looks before I send this off for review?

[David Grogan]

Comment on attachment 152815 [details]
Patch

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

LGTM

I didn't look too closely. It'd be good if Alec could also look.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:420
> +            keyGeneratorState = decodeInt(it->value().begin(), it->value().end());

Do you want to add some assert here? Or a fixme saying that this is unused for now but could be used?

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:506
> +    ok = putInt(m_currentTransaction.get(), keyGeneratorStateKey, 1);

Could you add a constant or comment about the bare 1?

[Alec Flett]

Comment on attachment 152815 [details]
Patch

sorry I have this habit of not commenting when it looks good...LGTM!

I was briefly happy to see that kMaxGeneratorValue was moving out to IDBKey, but I see that was just a typo :)

[Joshua Bell]

Created attachment 152854 [details]
Patch

[Joshua Bell]

(In reply to comment #11)
> Do you want to add some assert here? Or a fixme saying that this is unused for now but could be used?

Done (both)

> > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:506
> > +    ok = putInt(m_currentTransaction.get(), keyGeneratorStateKey, 1);
> 
> Could you add a constant or comment about the bare 1?

Add constant, used here and in above assert.

I also did s/State/CurrentNumber/g over the patch to better match the spec terminology.

tony@ - r?

[Tony Chang]

Comment on attachment 152854 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:50
> +const int64_t kKeyGeneratorInitialNumber = 1; // From the IndexedDB specification.

Nit: Normally we don't prefix constants or enum values with k.  KeyGeneratorInitialNumber is fine.  Hmm, I see you use k for constants in other places.  Maybe a follow up patch to remove all the k prefixes in IDB code?

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:680
> +            const char *p = it->key().begin();
> +            const char *limit = it->key().end();

Nit: * next to char.  E.g., const char* p =

[Joshua Bell]

Created attachment 152869 [details]
Patch for landing

[Joshua Bell]

(In reply to comment #15)
> Nit: Normally we don't prefix constants or enum values with k.  KeyGeneratorInitialNumber is fine.  Hmm, I see you use k for constants in other places.  Maybe a follow up patch to remove all the k prefixes in IDB code?

Fixed, and will do.

> > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:680
> > +            const char *p = it->key().begin();
> > +            const char *limit = it->key().end();
> 
> Nit: * next to char.  E.g., const char* p =

Yeah, that was an indentation of existing code. Fixed, and will do a global fixup in the followup patch.

[WebKit Review Bot]

Comment on attachment 152869 [details]
Patch for landing

Clearing flags on attachment: 152869

Committed r122909: <http://trac.webkit.org/changeset/122909>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.