153625 2016-01-28 17:20:03 -0800 Modern IDB: SQLite backend mismanages key generator values 2016-01-28 22:50:53 -0800 1 1 1 Unclassified WebKit WebCore Misc. Safari 9 Unspecified Unspecified RESOLVED FIXED P2 Normal --- 149117 153021 1 beidson beidson achristensen aestes alecflett commit-queue jsbell oldest_to_newest 1159955 0 beidson 2016-01-28 17:20:03 -0800 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. 1159956 1 beidson 2016-01-28 17:23:19 -0800 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. 1159957 2 270166 beidson 2016-01-28 17:23:31 -0800 Created attachment 270166 Patch v1 1160028 3 270166 aestes 2016-01-28 22:03:19 -0800 Comment on attachment 270166 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. 1160043 4 270166 commit-queue 2016-01-28 22:50:48 -0800 Comment on attachment 270166 Patch v1 Clearing flags on attachment: 270166 Committed r195801: <http://trac.webkit.org/changeset/195801> 1160044 5 commit-queue 2016-01-28 22:50:53 -0800 All reviewed patches have been landed. Closing bug. 270166 2016-01-28 17:23:31 -0800 2016-01-28 22:50:48 -0800 Patch v1 patch text/plain 5542 beidson ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCA2MmNmZDMzLi5kYjcwM2Q4IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTQgQEAKIDIwMTYtMDEt MjggIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgogCisgICAgICAgIE1vZGVybiBJ REI6IFNRTGl0ZSBiYWNrZW5kIG1pc21hbmFnZXMga2V5IGdlbmVyYXRvciB2YWx1ZXMuCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTM2MjUKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHBsYXRmb3JtL21h Yy13azEvVGVzdEV4cGVjdGF0aW9uczoKKworMjAxNi0wMS0yOCAgQnJhZHkgRWlkc29uICA8YmVp ZHNvbkBhcHBsZS5jb20+CisKICAgICAgICAgTW9kZXJuIElEQjogRml4IHNldmVyYWwgbW9yZSBw cm9ibGVtcyB3aXRoIG9iamVjdCBzdG9yZSBjaGFuZ2VzIGR1cmluZyBjdXJzb3IgaXRlcmF0aW9u IGluIFNRTGl0ZSBiYWNrZW5kLgogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTUzNjE2CiAKZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL3BsYXRmb3JtL21h Yy13azEvVGVzdEV4cGVjdGF0aW9ucyBiL0xheW91dFRlc3RzL3BsYXRmb3JtL21hYy13azEvVGVz dEV4cGVjdGF0aW9ucwppbmRleCBiYjA3NjYxLi40ZWI1YjgyIDEwMDY0NAotLS0gYS9MYXlvdXRU ZXN0cy9wbGF0Zm9ybS9tYWMtd2sxL1Rlc3RFeHBlY3RhdGlvbnMKKysrIGIvTGF5b3V0VGVzdHMv cGxhdGZvcm0vbWFjLXdrMS9UZXN0RXhwZWN0YXRpb25zCkBAIC0yNTYsMTkgKzI1NiwxMiBAQCBp bnNwZWN0b3Ivc2NyaXB0LXByb2ZpbGVyL2V2ZW50LXR5cGUtQVBJLmh0bWwKICMgU1FMaXRlIGJh Y2tlbmQgdGVzdHMgd2l0aCB0ZXh0IGZhaWx1cmVzCiBjcnlwdG8vc3VidGxlL3JzYS1pbmRleGVk ZGItbm9uLWV4cG9ydGFibGUuaHRtbCBbIEZhaWx1cmUgXQogZmFzdC9oaXN0b3J5L3BhZ2UtY2Fj aGUtaW5kZXhlZC1vcGVuZWQtZGIuaHRtbCBbIEZhaWx1cmUgXQotaW1wb3J0ZWQvdzNjL2luZGV4 ZWRkYi9pZGJkYXRhYmFzZV9kZWxldGVPYmplY3RTdG9yZTQtbm90X3JldXNlZC5odG0gWyBGYWls dXJlIF0KIGltcG9ydGVkL3czYy9pbmRleGVkZGIvaWRiaW5kZXgtbXVsdGllbnRyeS1iaWcuaHRt IFsgRmFpbHVyZSBdCi1pbXBvcnRlZC93M2MvaW5kZXhlZGRiL2tleWdlbmVyYXRvci1jb25zdHJh aW50ZXJyb3IuaHRtIFsgRmFpbHVyZSBdCi1pbXBvcnRlZC93M2MvaW5kZXhlZGRiL2tleWdlbmVy YXRvci5odG0gWyBGYWlsdXJlIF0KLWltcG9ydGVkL3czYy9pbmRleGVkZGIvdHJhbnNhY3Rpb24t cmVxdWVzdHF1ZXVlLmh0bSBbIEZhaWx1cmUgXQogc3RvcmFnZS9pbmRleGVkZGIvY3Vyc29yLWNv bnRpbnVlLXZhbGlkaXR5Lmh0bWwgWyBGYWlsdXJlIF0KIHN0b3JhZ2UvaW5kZXhlZGRiL2N1cnNv ci1wcmltYXJ5LWtleS1vcmRlci5odG1sIFsgRmFpbHVyZSBdCiBzdG9yYWdlL2luZGV4ZWRkYi9n ZXQta2V5cmFuZ2UuaHRtbCBbIEZhaWx1cmUgXQotc3RvcmFnZS9pbmRleGVkZGIvaW5kZXgtZHVw bGljYXRlLWtleXBhdGhzLmh0bWwgWyBGYWlsdXJlIF0KLXN0b3JhZ2UvaW5kZXhlZGRiL2tleS1n ZW5lcmF0b3IuaHRtbCBbIEZhaWx1cmUgXQogc3RvcmFnZS9pbmRleGVkZGIvbW9kZXJuL2dldC1r ZXlyYW5nZS5odG1sIFsgRmFpbHVyZSBdCiBzdG9yYWdlL2luZGV4ZWRkYi9tb2Rlcm4vaW5kZXgt My5odG1sIFsgRmFpbHVyZSBdCi1zdG9yYWdlL2luZGV4ZWRkYi9vYmplY3RzdG9yZS1hdXRvaW5j cmVtZW50Lmh0bWwgWyBGYWlsdXJlIF0KIAogIyBTUUxpdGUgYmFja2VuZCB0ZXN0cyB0aGF0IHRp bWVvdXQKIHN0b3JhZ2UvaW5kZXhlZGRiL21vZGVybi90cmFuc2FjdGlvbi1zY2hlZHVsZXItMS5o dG1sIFsgU2tpcCBdCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9Tb3Vy Y2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggM2NiMjQ5Mi4uNjhkNTE5MyAxMDA2NDQKLS0tIGEv U291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpA QCAtMSw1ICsxLDIzIEBACiAyMDE2LTAxLTI4ICBCcmFkeSBFaWRzb24gIDxiZWlkc29uQGFwcGxl LmNvbT4KIAorICAgICAgICBNb2Rlcm4gSURCOiBTUUxpdGUgYmFja2VuZCBtaXNtYW5hZ2VzIGtl eSBnZW5lcmF0b3IgdmFsdWVzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTUzNjI1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgTm8gbmV3IHRlc3RzIChNYW55IGZhaWxpbmcgdGVzdHMgcGFzcywgYSBmZXcg Z2V0IGNsb3NlcikuCisKKyAgICAgICAgVGhlcmUncyBtaXhlZCBhc3N1bXB0aW9ucyBhYm91dCB3 aGV0aGVyIHRoZSB2YWx1ZSBzdG9yZWQgaXMgdGhlIGN1cnJlbnQgdmFsdWUgb3IgdGhlIG5leHQg dmFsdWUuCisKKyAgICAgICAgRml4aW5nIHRob3NlIGFzc3VtcHRpb25zIGZpeGVzIHRlc3RzLgor ICAgICAgICAKKyAgICAgICAgKiBNb2R1bGVzL2luZGV4ZWRkYi9zZXJ2ZXIvU1FMaXRlSURCQmFj a2luZ1N0b3JlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OklEQlNlcnZlcjo6U1FMaXRlSURCQmFj a2luZ1N0b3JlOjpnZW5lcmF0ZUtleU51bWJlcik6IFN0b3JlL3JldHJpZXZlIHRoZSBjb3JyZWN0 IHZhbHVlLgorICAgICAgICAoV2ViQ29yZTo6SURCU2VydmVyOjpTUUxpdGVJREJCYWNraW5nU3Rv cmU6OnJldmVydEdlbmVyYXRlZEtleU51bWJlcik6IERpdHRvLgorICAgICAgICAoV2ViQ29yZTo6 SURCU2VydmVyOjpTUUxpdGVJREJCYWNraW5nU3RvcmU6Om1heWJlVXBkYXRlS2V5R2VuZXJhdG9y TnVtYmVyKTogRGl0dG8uCisKKzIwMTYtMDEtMjggIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBw bGUuY29tPgorCiAgICAgICAgIE1vZGVybiBJREI6IEZpeCBzZXZlcmFsIG1vcmUgcHJvYmxlbXMg d2l0aCBvYmplY3Qgc3RvcmUgY2hhbmdlcyBkdXJpbmcgY3Vyc29yIGl0ZXJhdGlvbiBpbiBTUUxp dGUgYmFja2VuZC4KICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE1MzYxNgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL2luZGV4ZWRk Yi9zZXJ2ZXIvU1FMaXRlSURCQmFja2luZ1N0b3JlLmNwcCBiL1NvdXJjZS9XZWJDb3JlL01vZHVs ZXMvaW5kZXhlZGRiL3NlcnZlci9TUUxpdGVJREJCYWNraW5nU3RvcmUuY3BwCmluZGV4IGYxZWY4 NWUuLjBlMTMwNWQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvaW5kZXhlZGRi L3NlcnZlci9TUUxpdGVJREJCYWNraW5nU3RvcmUuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL01v ZHVsZXMvaW5kZXhlZGRiL3NlcnZlci9TUUxpdGVJREJCYWNraW5nU3RvcmUuY3BwCkBAIC0xMzU2 LDcgKzEzNTYsNyBAQCBJREJFcnJvciBTUUxpdGVJREJCYWNraW5nU3RvcmU6OmdlbmVyYXRlS2V5 TnVtYmVyKGNvbnN0IElEQlJlc291cmNlSWRlbnRpZmllciYgdAogICAgIGlmICghZXJyb3IuaXNO dWxsKCkpCiAgICAgICAgIHJldHVybiBlcnJvcjsKIAotICAgIGlmIChjdXJyZW50VmFsdWUgPiBt YXhHZW5lcmF0b3JWYWx1ZSkKKyAgICBpZiAoY3VycmVudFZhbHVlICsgMSA+IG1heEdlbmVyYXRv clZhbHVlKQogICAgICAgICByZXR1cm4geyBJREJEYXRhYmFzZUV4Y2VwdGlvbjo6Q29uc3RyYWlu dEVycm9yLCAiQ2Fubm90IGdlbmVyYXRlIG5ldyBrZXkgdmFsdWUgb3ZlciAyXjUzIGZvciBvYmpl Y3Qgc3RvcmUgb3BlcmF0aW9uIiB9OwogCiAgICAgZ2VuZXJhdGVkS2V5ID0gY3VycmVudFZhbHVl ICsgMTsKQEAgLTEzNjUsNyArMTM2NSw3IEBAIElEQkVycm9yIFNRTGl0ZUlEQkJhY2tpbmdTdG9y ZTo6Z2VuZXJhdGVLZXlOdW1iZXIoY29uc3QgSURCUmVzb3VyY2VJZGVudGlmaWVyJiB0CiAKIElE QkVycm9yIFNRTGl0ZUlEQkJhY2tpbmdTdG9yZTo6cmV2ZXJ0R2VuZXJhdGVkS2V5TnVtYmVyKGNv bnN0IElEQlJlc291cmNlSWRlbnRpZmllciYgdHJhbnNhY3Rpb25JZGVudGlmaWVyLCB1aW50NjRf dCBvYmplY3RTdG9yZUlELCB1aW50NjRfdCBuZXdLZXlOdW1iZXIpCiB7Ci0gICAgTE9HKEluZGV4 ZWREQiwgIlNRTGl0ZUlEQkJhY2tpbmdTdG9yZTo6cmV2ZXJ0R2VuZXJhdGVkS2V5TnVtYmVyIik7 CisgICAgTE9HKEluZGV4ZWREQiwgIlNRTGl0ZUlEQkJhY2tpbmdTdG9yZTo6cmV2ZXJ0R2VuZXJh dGVkS2V5TnVtYmVyIC0gb2JqZWN0IHN0b3JlICUiIFBSSXU2NCAiLCByZXZlcnRlZCBudW1iZXIg JSIgUFJJdTY0LCBvYmplY3RTdG9yZUlELCBuZXdLZXlOdW1iZXIpOwogCiAgICAgQVNTRVJUKG1f c3FsaXRlREIpOwogICAgIEFTU0VSVChtX3NxbGl0ZURCLT5pc09wZW4oKSk7CkBAIC0xMzgwLDcg KzEzODAsOCBAQCBJREJFcnJvciBTUUxpdGVJREJCYWNraW5nU3RvcmU6OnJldmVydEdlbmVyYXRl ZEtleU51bWJlcihjb25zdCBJREJSZXNvdXJjZUlkZW50aQogICAgICAgICByZXR1cm4geyBJREJE YXRhYmFzZUV4Y2VwdGlvbjo6VW5rbm93bkVycm9yLCBBU0NJSUxpdGVyYWwoIkF0dGVtcHQgdG8g cmV2ZXJ0IGtleSBnZW5lcmF0b3IgdmFsdWUgaW4gYSByZWFkLW9ubHkgdHJhbnNhY3Rpb24iKSB9 OwogICAgIH0KIAotICAgIHJldHVybiB1bmNoZWNrZWRTZXRLZXlHZW5lcmF0b3JWYWx1ZShvYmpl Y3RTdG9yZUlELCBuZXdLZXlOdW1iZXIpOworICAgIEFTU0VSVChuZXdLZXlOdW1iZXIpOworICAg IHJldHVybiB1bmNoZWNrZWRTZXRLZXlHZW5lcmF0b3JWYWx1ZShvYmplY3RTdG9yZUlELCBuZXdL ZXlOdW1iZXIgLSAxKTsKIH0KIAogSURCRXJyb3IgU1FMaXRlSURCQmFja2luZ1N0b3JlOjptYXli ZVVwZGF0ZUtleUdlbmVyYXRvck51bWJlcihjb25zdCBJREJSZXNvdXJjZUlkZW50aWZpZXImIHRy YW5zYWN0aW9uSWRlbnRpZmllciwgdWludDY0X3Qgb2JqZWN0U3RvcmVJRCwgZG91YmxlIG5ld0tl eU51bWJlcikKQEAgLTE0MTQsNyArMTQxNSw3IEBAIElEQkVycm9yIFNRTGl0ZUlEQkJhY2tpbmdT dG9yZTo6bWF5YmVVcGRhdGVLZXlHZW5lcmF0b3JOdW1iZXIoY29uc3QgSURCUmVzb3VyY2VJCiAK ICAgICBBU1NFUlQobmV3S2V5SW50ZWdlciA+IHVpbnQ2NF90KG5ld0tleU51bWJlcikpOwogCi0g ICAgcmV0dXJuIHVuY2hlY2tlZFNldEtleUdlbmVyYXRvclZhbHVlKG9iamVjdFN0b3JlSUQsIG5l d0tleUludGVnZXIpOworICAgIHJldHVybiB1bmNoZWNrZWRTZXRLZXlHZW5lcmF0b3JWYWx1ZShv YmplY3RTdG9yZUlELCBuZXdLZXlJbnRlZ2VyIC0gMSk7CiB9CiAKIElEQkVycm9yIFNRTGl0ZUlE QkJhY2tpbmdTdG9yZTo6b3BlbkN1cnNvcihjb25zdCBJREJSZXNvdXJjZUlkZW50aWZpZXImIHRy YW5zYWN0aW9uSWRlbnRpZmllciwgY29uc3QgSURCQ3Vyc29ySW5mbyYgaW5mbywgSURCR2V0UmVz dWx0JiByZXN1bHQpCg==