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
Brady Eidson
2016-01-28 17:20:03 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. Created attachment 270166 [details]
Patch v1
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 on attachment 270166 [details] Patch v1 Clearing flags on attachment: 270166 Committed r195801: <http://trac.webkit.org/changeset/195801> All reviewed patches have been landed. Closing bug. |