RESOLVED FIXED 153625
Modern IDB: SQLite backend mismanages key generator values
https://bugs.webkit.org/show_bug.cgi?id=153625
Summary Modern IDB: SQLite backend mismanages key generator values
Brady Eidson
Reported 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.
Attachments
Patch v1 (5.41 KB, patch)
2016-01-28 17:23 PST, Brady Eidson
no flags
Brady Eidson
Comment 1 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.
Brady Eidson
Comment 2 2016-01-28 17:23:31 PST
Created attachment 270166 [details] Patch v1
Andy Estes
Comment 3 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.
WebKit Commit Bot
Comment 4 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>
WebKit Commit Bot
Comment 5 2016-01-28 22:50:53 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.