229886 2021-09-03 12:40:16 -0700 Regression(r277571) Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() from ITP fails 2023-07-18 09:02:48 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=259284 InRadar P2 Normal --- 1 cdumez cdumez achristensen darin ggaren katherine_cheney sam webkit-bug-importer oldest_to_newest 1790293 0 cdumez 2021-09-03 12:40:16 -0700 Call to SQLiteDatabase::turnOnIncrementalAutoVacuum() from ITP fails since r277571 and logs: 0x104de9900 - ResourceLoadStatisticsDatabaseStore::turnOnIncrementalAutoVacuum failed, error message: not an error 1790295 1 cdumez 2021-09-03 12:40:33 -0700 <rdar://82581507> 1790299 2 437299 cdumez 2021-09-03 12:44:32 -0700 Created attachment 437299 Patch 1790339 3 437299 darin 2021-09-03 13:42:43 -0700 Comment on attachment 437299 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437299&action=review > Source/WebCore/platform/sql/SQLiteDatabase.cpp:630 > int autoVacuumMode = 0; > - if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) > + int error = 0; > + if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) { > autoVacuumMode = statement->columnInt(0); > - int error = lastError(); > + error = lastError(); > + } Seems like we can straighten out the logic a little. If we can’t prepare the statement, we return false, but for some reason we implement that by setting error to "0" and then comparing it with SQLITE_ROW. I would write: auto statement = prepareStatement("PRAGMA auto_vacuum"_s); if (!statement) return false; // <Move here and/or improve long confusing comment about error codes that does not even mention SQLITE_ROW>? auto autoVacuumMode = statement->columnInt(0); if (lastError() != SQLITE_ROW) return false; I like that better than having a local variable named error. The code below also doesn’t need a local variable. return lastError() == SQLITE_OK; 1790340 4 cdumez 2021-09-03 13:44:26 -0700 (In reply to Darin Adler from comment #3) > Comment on attachment 437299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=437299&action=review > > > Source/WebCore/platform/sql/SQLiteDatabase.cpp:630 > > int autoVacuumMode = 0; > > - if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) > > + int error = 0; > > + if (auto statement = prepareStatement("PRAGMA auto_vacuum"_s)) { > > autoVacuumMode = statement->columnInt(0); > > - int error = lastError(); > > + error = lastError(); > > + } > > Seems like we can straighten out the logic a little. If we can’t prepare the > statement, we return false, but for some reason we implement that by setting > error to "0" and then comparing it with SQLITE_ROW. I would write: > > auto statement = prepareStatement("PRAGMA auto_vacuum"_s); > if (!statement) > return false; > > // <Move here and/or improve long confusing comment about error codes > that does not even mention SQLITE_ROW>? > auto autoVacuumMode = statement->columnInt(0); > if (lastError() != SQLITE_ROW) > return false; > > I like that better than having a local variable named error. The code below > also doesn’t need a local variable. > > return lastError() == SQLITE_OK; Your proposal is extending the lifetime of the statement. I was trying to keep the same lifetime as before r277571 (which is why I had added the scope in the first place). Maybe it doesn't matter though. 1790346 5 437308 cdumez 2021-09-03 13:52:39 -0700 Created attachment 437308 Patch 1790374 6 437308 achristensen 2021-09-03 14:43:39 -0700 Comment on attachment 437308 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437308&action=review > Source/WebCore/platform/sql/SQLiteDatabase.cpp:625 > + auto statement = prepareStatement("PRAGMA auto_vacuum"_s); I'm not sure if there is a consequence to having this statement call finalize after the statement inside executeCommand or runVacuumCommand, but I think this might change behavior more than we want it to. 1790379 7 437308 cdumez 2021-09-03 14:46:26 -0700 Comment on attachment 437308 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437308&action=review >> Source/WebCore/platform/sql/SQLiteDatabase.cpp:625 >> + auto statement = prepareStatement("PRAGMA auto_vacuum"_s); > > I'm not sure if there is a consequence to having this statement call finalize after the statement inside executeCommand or runVacuumCommand, but I think this might change behavior more than we want it to. I thought about it and I personally don't see how it changes behavior. Yes, 2 statements will live at the same time but I don't see any issue with that (except that we're using a very tiny amount of extra memory for a little bit longer). 1790405 8 ews-feeder 2021-09-03 15:16:08 -0700 Committed r282030 (241334@main): <https://commits.webkit.org/241334@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437308. 437299 2021-09-03 12:44:32 -0700 2021-09-03 13:52:37 -0700 Patch bug-229886-20210903124431.patch text/plain 2385 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjgyMDA5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDYxYzA3ZjVkMjgwZWQw YTdiZmFlYTNhODliYjIyNDlhZDhjYzgwOS4uOTQyZGU0NDg3YzkzNGM1N2MwNzBjOGYyYjM3MGNl ZDhlZTBiM2YzYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDIxLTA5LTAzICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgUmVncmVzc2lvbihyMjc3NTcx KSBDYWxsIHRvIFNRTGl0ZURhdGFiYXNlOjp0dXJuT25JbmNyZW1lbnRhbEF1dG9WYWN1dW0oKSBm cm9tIElUUCBmYWlscworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MjI5ODg2CisgICAgICAgIDxyZGFyOi8vODI1ODE1MDc+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQmVmb3JlIHIyNzc1NzEsIGBsYXN0RXJy b3IoKWAgd2FzIGdldHRpbmcgY2FsbGVkIGFmdGVyIHRoZSBjYWxsIHRvIGBzdGF0ZW1lbnQtPmNv bHVtbkludCgwKWAKKyAgICAgICAgYW5kICpiZWZvcmUqIHRoZSBjYWxsIHRvIGBzdGF0ZW1lbnQu ZmluYWxpemUoKWAuIEFmdGVyIHIyNzc1NzEsIHRoZSBjYWxsIHRvIGBzdGF0ZW1lbnQuZmluYWxp emUoKWAKKyAgICAgICAgYmVjYW1lIGltcGxpY2l0LCBhbmQgZ2V0cyBjYWxsZWQgd2hlbiBgc3Rh dGVtZW50YCBkb2VzIG91dCBvZiBzY29wZS4gQXMgYSByZXN1bHQsIGBsYXN0RXJyb3IoKWAKKyAg ICAgICAgd2FzIGdldHRpbmcgY2FsbGVkICphZnRlciogdGhlIHN0YXRlbWVudCBmaW5hbGl6YXRp b24gaW5zdGVhZCBvZiBhZnRlci4gVGhpcyBpcyBhbiBpc3N1ZSBiZWNhdXNlCisgICAgICAgIHRo ZSBjb2RlIGV4cGVjdHMgdGhlIGxhc3QgZXJyb3IgdG8gYmUgZm9yIGBzdGF0ZW1lbnQtPmNvbHVt bkludCgwKWAgYW5kIHRodXMgZXhwZWN0cyBTUUxJVEVfUk9XLgorICAgICAgICBJIG1vdmVkIHRo ZSBgbGFzdEVycm9yKClgIGNhbGwgc28gdGhhdCBpdCBub3cgYWZ0ZXIgYmV0d2VlbiBgc3RhdGVt ZW50LT5jb2x1bW5JbnQoMClgIGFuZCB0aGUKKyAgICAgICAgaW1wbGljaXQgc3RhdGVtZW50IGZp bmFsaXphdGlvbiwgbGlrZSBpdCB1c2VkIHRvLgorCisgICAgICAgICogcGxhdGZvcm0vc3FsL1NR TGl0ZURhdGFiYXNlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNRTGl0ZURhdGFiYXNlOjp0dXJu T25JbmNyZW1lbnRhbEF1dG9WYWN1dW0pOgorCiAyMDIxLTA5LTAzICBaaXJhbiBTdW4gIDx6c3Vu QGlnYWxpYS5jb20+CiAKICAgICAgICAgUkVHUkVTU0lPTihyMjgwMDc4KTogYnJva2UgZmFzdC9p bWFnZXMvZXhpZi1vcmllbnRhdGlvbi1jb21wb3NpdGVkLmh0bWwgb24gd2luZG93cwpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0ZURhdGFiYXNlLmNwcCBiL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5jcHAKaW5kZXggNzQzZWYz ZGM4NDExZmU5ZTA5ODNlODA3NTEyNWFjM2JhYTEyNTQ4ZC4uZTA2Zjg5MGU0Y2VlNjRmYjEzZmYy MGZiNDM1NmNjNDgxMjcyZDY2MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v c3FsL1NRTGl0ZURhdGFiYXNlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9zcWwv U1FMaXRlRGF0YWJhc2UuY3BwCkBAIC02MjMsOSArNjIzLDExIEBAIGJvb2wgU1FMaXRlRGF0YWJh c2U6OmlzQXV0b0NvbW1pdE9uKCkgY29uc3QKIGJvb2wgU1FMaXRlRGF0YWJhc2U6OnR1cm5Pbklu Y3JlbWVudGFsQXV0b1ZhY3V1bSgpCiB7CiAgICAgaW50IGF1dG9WYWN1dW1Nb2RlID0gMDsKLSAg ICBpZiAoYXV0byBzdGF0ZW1lbnQgPSBwcmVwYXJlU3RhdGVtZW50KCJQUkFHTUEgYXV0b192YWN1 dW0iX3MpKQorICAgIGludCBlcnJvciA9IDA7CisgICAgaWYgKGF1dG8gc3RhdGVtZW50ID0gcHJl cGFyZVN0YXRlbWVudCgiUFJBR01BIGF1dG9fdmFjdXVtIl9zKSkgewogICAgICAgICBhdXRvVmFj dXVtTW9kZSA9IHN0YXRlbWVudC0+Y29sdW1uSW50KDApOwotICAgIGludCBlcnJvciA9IGxhc3RF cnJvcigpOworICAgICAgICBlcnJvciA9IGxhc3RFcnJvcigpOworICAgIH0KIAogICAgIC8vIENo ZWNrIGlmIHdlIGdvdCBhbiBlcnJvciB3aGlsZSB0cnlpbmcgdG8gZ2V0IHRoZSB2YWx1ZSBvZiB0 aGUgYXV0b192YWN1dW0gZmxhZy4KICAgICAvLyBJZiB3ZSBnb3QgYSBTUUxJVEVfQlVTWSBlcnJv ciwgdGhlbiB0aGVyZSdzIHByb2JhYmx5IGFub3RoZXIgdHJhbnNhY3Rpb24gaW4K 437308 2021-09-03 13:52:39 -0700 2021-09-03 15:16:09 -0700 Patch bug-229886-20210903135238.patch text/plain 3205 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjgyMDIwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggM2IxNDJjYTY1N2ZhZDFm YWY5NmVkZTA2MDk3NTY4YzMzZjQ2NjllMy4uNTQ0OWJkYzVmYzRhNzAzMzMyYzMyMzE0MzMxYTgz MzU2YTI2MmVkMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDIxLTA5LTAzICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgUmVncmVzc2lvbihyMjc3NTcx KSBDYWxsIHRvIFNRTGl0ZURhdGFiYXNlOjp0dXJuT25JbmNyZW1lbnRhbEF1dG9WYWN1dW0oKSBm cm9tIElUUCBmYWlscworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MjI5ODg2CisgICAgICAgIDxyZGFyOi8vODI1ODE1MDc+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgQWxleCBDaHJpc3RlbnNlbi4KKworICAgICAgICBCZWZvcmUgcjI3NzU3MSwgYGxhc3RF cnJvcigpYCB3YXMgZ2V0dGluZyBjYWxsZWQgYWZ0ZXIgdGhlIGNhbGwgdG8gYHN0YXRlbWVudC0+ Y29sdW1uSW50KDApYAorICAgICAgICBhbmQgKmJlZm9yZSogdGhlIGNhbGwgdG8gYHN0YXRlbWVu dC5maW5hbGl6ZSgpYC4gQWZ0ZXIgcjI3NzU3MSwgdGhlIGNhbGwgdG8gYHN0YXRlbWVudC5maW5h bGl6ZSgpYAorICAgICAgICBiZWNhbWUgaW1wbGljaXQsIGFuZCBnZXRzIGNhbGxlZCB3aGVuIGBz dGF0ZW1lbnRgIGRvZXMgb3V0IG9mIHNjb3BlLiBBcyBhIHJlc3VsdCwgYGxhc3RFcnJvcigpYAor ICAgICAgICB3YXMgZ2V0dGluZyBjYWxsZWQgKmFmdGVyKiB0aGUgc3RhdGVtZW50IGZpbmFsaXph dGlvbiBpbnN0ZWFkIG9mIGFmdGVyLiBUaGlzIGlzIGFuIGlzc3VlIGJlY2F1c2UKKyAgICAgICAg dGhlIGNvZGUgZXhwZWN0cyB0aGUgbGFzdCBlcnJvciB0byBiZSBmb3IgYHN0YXRlbWVudC0+Y29s dW1uSW50KDApYCBhbmQgdGh1cyBleHBlY3RzIFNRTElURV9ST1cuCisgICAgICAgIEkgbW92ZWQg dGhlIGBsYXN0RXJyb3IoKWAgY2FsbCBzbyB0aGF0IGl0IG5vdyBhZnRlciBiZXR3ZWVuIGBzdGF0 ZW1lbnQtPmNvbHVtbkludCgwKWAgYW5kIHRoZQorICAgICAgICBpbXBsaWNpdCBzdGF0ZW1lbnQg ZmluYWxpemF0aW9uLCBsaWtlIGl0IHVzZWQgdG8uCisKKyAgICAgICAgKiBwbGF0Zm9ybS9zcWwv U1FMaXRlRGF0YWJhc2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U1FMaXRlRGF0YWJhc2U6OnR1 cm5PbkluY3JlbWVudGFsQXV0b1ZhY3V1bSk6CisKIDIwMjEtMDktMDMgIEFkaXR5YSBLZWVydGhp ICA8YWtlZXJ0aGlAYXBwbGUuY29tPgogCiAgICAgICAgIGlmcmFtZXMgc2hvdWxkIGdldCBhbiBv cGFxdWUgYmFja2dyb3VuZCB3aGVuIHRoZSBlbWJlZGRpbmcgZWxlbWVudCBhbmQgZW1iZWRkZWQg cm9vdCBjb2xvci1zY2hlbWVzIGRvIG5vdCBtYXRjaApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vc3FsL1NRTGl0ZURhdGFiYXNlLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5jcHAKaW5kZXggNzQzZWYzZGM4NDExZmU5ZTA5ODNlODA3 NTEyNWFjM2JhYTEyNTQ4ZC4uMTg0MzgzOTk3ZDU1ZDRjNTZjMmIxOTIyNDFlNjkxNDY1MGU0ZTA4 MyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0ZURhdGFiYXNl LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9zcWwvU1FMaXRlRGF0YWJhc2UuY3Bw CkBAIC02MjIsMTAgKzYyMiwxMSBAQCBib29sIFNRTGl0ZURhdGFiYXNlOjppc0F1dG9Db21taXRP bigpIGNvbnN0CiAKIGJvb2wgU1FMaXRlRGF0YWJhc2U6OnR1cm5PbkluY3JlbWVudGFsQXV0b1Zh Y3V1bSgpCiB7Ci0gICAgaW50IGF1dG9WYWN1dW1Nb2RlID0gMDsKLSAgICBpZiAoYXV0byBzdGF0 ZW1lbnQgPSBwcmVwYXJlU3RhdGVtZW50KCJQUkFHTUEgYXV0b192YWN1dW0iX3MpKQotICAgICAg ICBhdXRvVmFjdXVtTW9kZSA9IHN0YXRlbWVudC0+Y29sdW1uSW50KDApOwotICAgIGludCBlcnJv ciA9IGxhc3RFcnJvcigpOworICAgIGF1dG8gc3RhdGVtZW50ID0gcHJlcGFyZVN0YXRlbWVudCgi UFJBR01BIGF1dG9fdmFjdXVtIl9zKTsKKyAgICBpZiAoIXN0YXRlbWVudCkKKyAgICAgICAgcmV0 dXJuIGZhbHNlOworCisgICAgaW50IGF1dG9WYWN1dW1Nb2RlID0gc3RhdGVtZW50LT5jb2x1bW5J bnQoMCk7CiAKICAgICAvLyBDaGVjayBpZiB3ZSBnb3QgYW4gZXJyb3Igd2hpbGUgdHJ5aW5nIHRv IGdldCB0aGUgdmFsdWUgb2YgdGhlIGF1dG9fdmFjdXVtIGZsYWcuCiAgICAgLy8gSWYgd2UgZ290 IGEgU1FMSVRFX0JVU1kgZXJyb3IsIHRoZW4gdGhlcmUncyBwcm9iYWJseSBhbm90aGVyIHRyYW5z YWN0aW9uIGluCkBAIC02MzMsNyArNjM0LDcgQEAgYm9vbCBTUUxpdGVEYXRhYmFzZTo6dHVybk9u SW5jcmVtZW50YWxBdXRvVmFjdXVtKCkKICAgICAvLyBhdXRvX3ZhY3V1bSBmbGFnIGFuZCB0cnkg dG8gc2V0IGl0IHRvIElOQ1JFTUVOVEFMIHRoZSBuZXh0IHRpbWUgd2Ugb3BlbiB0aGlzCiAgICAg Ly8gZGF0YWJhc2UuIElmIHRoZSBlcnJvciBpcyBub3QgU1FMSVRFX0JVU1ksIHRoZW4gd2UgcHJv YmFibHkgcmFuIGludG8gYSBtb3JlCiAgICAgLy8gc2VyaW91cyBwcm9ibGVtIGFuZCBzaG91bGQg cmV0dXJuIGZhbHNlICh0byBsb2cgYW4gZXJyb3IgbWVzc2FnZSkuCi0gICAgaWYgKGVycm9yICE9 IFNRTElURV9ST1cpCisgICAgaWYgKGxhc3RFcnJvcigpICE9IFNRTElURV9ST1cpCiAgICAgICAg IHJldHVybiBmYWxzZTsKIAogICAgIHN3aXRjaCAoYXV0b1ZhY3V1bU1vZGUpIHsKQEAgLTY0Niw4 ICs2NDcsNyBAQCBib29sIFNRTGl0ZURhdGFiYXNlOjp0dXJuT25JbmNyZW1lbnRhbEF1dG9WYWN1 dW0oKQogICAgICAgICBpZiAoIWV4ZWN1dGVDb21tYW5kKCJQUkFHTUEgYXV0b192YWN1dW0gPSAy Il9zKSkKICAgICAgICAgICAgIHJldHVybiBmYWxzZTsKICAgICAgICAgcnVuVmFjdXVtQ29tbWFu ZCgpOwotICAgICAgICBlcnJvciA9IGxhc3RFcnJvcigpOwotICAgICAgICByZXR1cm4gKGVycm9y ID09IFNRTElURV9PSyk7CisgICAgICAgIHJldHVybiBsYXN0RXJyb3IoKSA9PSBTUUxJVEVfT0s7 CiAgICAgfQogfQogCg==