Bug 153625

Summary: Modern IDB: SQLite backend mismanages key generator values
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, alecflett, commit-queue, jsbell
Priority: P2    
Version: Safari 9   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149117, 153021    
Attachments:
Description Flags
Patch v1 none

Description Brady Eidson 2016-01-28 17:20:03 PST
Modern IDB: SQLite backend mismanages key generator values

There's mixed assumptions about whether the value stored is the current value or the next value.

Fixing those fixes tests.
Comment 1 Brady Eidson 2016-01-28 17:23:19 PST
This likely won't apply because it's waiting on https://bugs.webkit.org/show_bug.cgi?id=153623

I'll mark it as a patch and put up for review once the other bug is resolved.
Comment 2 Brady Eidson 2016-01-28 17:23:31 PST
Created attachment 270166 [details]
Patch v1
Comment 3 Andy Estes 2016-01-28 22:03:19 PST
Comment on attachment 270166 [details]
Patch v1

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

r=me. My comments are mostly high-level, and could be addressed in a follow-up if you wish.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1362
> -    if (currentValue > maxGeneratorValue)
> +    if (currentValue + 1 > maxGeneratorValue)
>          return { IDBDatabaseException::ConstraintError, "Cannot generate new key value over 2^53 for object store operation" };
>  
>      generatedKey = currentValue + 1;

It might be a somewhat more clear to create a new local called nextGeneratedKey instead of incrementing currentValue in two places. I also don't love that we alternatively name a key generator value with terms containing "value", "key", and "number". Seems like a little more consistency would make this read easier.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1418
> -    return uncheckedSetKeyGeneratorValue(objectStoreID, newKeyInteger);
> +    return uncheckedSetKeyGeneratorValue(objectStoreID, newKeyInteger - 1);

This -1 is mysterious to me, even more so because the equivalent code in the memory backing store doesn't do this. I'm sure it's right given the test progressions, but I feel like this could use some clarity.
Comment 4 WebKit Commit Bot 2016-01-28 22:50:48 PST
Comment on attachment 270166 [details]
Patch v1

Clearing flags on attachment: 270166

Committed r195801: <http://trac.webkit.org/changeset/195801>
Comment 5 WebKit Commit Bot 2016-01-28 22:50:53 PST
All reviewed patches have been landed.  Closing bug.