47859 2010-10-18 16:09:54 -0700 sqlite: show extended error codes in error logs 2010-10-21 08:49:19 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 evan evan commit-queue ddkilzer dumi jorlow tony oldest_to_newest 295816 0 evan 2010-10-18 16:09:54 -0700 sqlite: show extended error codes in error logs 295819 1 71098 evan 2010-10-18 16:12:26 -0700 Created attachment 71098 Patch 295821 2 evan 2010-10-18 16:13:31 -0700 This helped me track down http://code.google.com/p/chromium/issues/detail?id=59420 295891 3 71098 tony 2010-10-18 18:33:31 -0700 Comment on attachment 71098 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71098&action=review > WebCore/platform/sql/SQLiteDatabase.cpp:76 > + m_lastError = sqlite3_extended_result_codes(m_db, 1); I'm not sure why m_lastError is a member variable. It doesn't appear to be used other than in the open function. Also, it looks like we only get here when m_lastError == SQLITE_OK. Will sqlite3_extended_result_codes provide information when SQLITE_OK? It seems weird that sqlite3_extended_result_codes returns the previous error. 296318 4 71098 ddkilzer 2010-10-19 12:04:06 -0700 Comment on attachment 71098 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71098&action=review r=me, but please address the questions below before committing. >> WebCore/platform/sql/SQLiteDatabase.cpp:76 >> + m_lastError = sqlite3_extended_result_codes(m_db, 1); > > I'm not sure why m_lastError is a member variable. It doesn't appear to be used other than in the open function. > > Also, it looks like we only get here when m_lastError == SQLITE_OK. Will sqlite3_extended_result_codes provide information when SQLITE_OK? It seems weird that sqlite3_extended_result_codes returns the previous error. Making m_lastError a local variable should be a separate patch, but I agree that it doesn't need to be a member variable. The sqlite3_extended_result_codes() function enables extended error codes for future database operations. My only concern is whether enabling extended error codes has any performance impact. (I suspect it doesn't because the error codes are just integers.) > WebCore/platform/sql/SQLiteStatement.cpp:108 > + error, m_query.ascii().data(), error, sqlite3_errmsg(m_database.sqlite3Handle())); Isn't printing 'error' twice here redundant? 296906 5 71313 evan 2010-10-20 12:02:38 -0700 Created attachment 71313 Patch 296941 6 71313 tony 2010-10-20 12:40:35 -0700 Comment on attachment 71313 Patch Carrying over ddkilzer's previous r+. 297322 7 71313 commit-queue 2010-10-21 02:53:48 -0700 Comment on attachment 71313 Patch Clearing flags on attachment: 71313 Committed r70219: <http://trac.webkit.org/changeset/70219> 297323 8 commit-queue 2010-10-21 02:53:54 -0700 All reviewed patches have been landed. Closing bug. 297393 9 71313 ddkilzer 2010-10-21 05:59:50 -0700 Comment on attachment 71313 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71313&action=review > WebCore/platform/sql/SQLiteDatabase.cpp:80 > + if (sqlite3_extended_result_codes(m_db, 1) != SQLITE_OK) { > + LOG_ERROR("SQLite database error when enabling extended errors - %s", sqlite3_errmsg(m_db)); > + sqlite3_close(m_db); > + m_db = 0; > + return false; > + } I'm not sure if a failure to enable extended result codes should cause the database open to fail, but I suppose it will be interesting to know when it doesn't work. No change is required--I just noticed this after Tony re-added my r+ to this patch. 297447 10 evan 2010-10-21 08:49:19 -0700 Heh, just looked at the sqlite source. int sqlite3_extended_result_codes(sqlite3 *db, int onoff){ sqlite3_mutex_enter(db->mutex); db->errMask = onoff ? 0xffffffff : 0xff; sqlite3_mutex_leave(db->mutex); return SQLITE_OK; } So I guess that is dead code. :\ I imagine they return the error code to leave it open to potentially returning errors in the future. I wonder what sort of errors they could be. 71098 2010-10-18 16:12:26 -0700 2010-10-20 12:02:34 -0700 Patch bug-47859-20101018161251.patch text/plain 2645 evan ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NTRmZjAyMjA3YTVjNjk4MTY0NmIyN2IwNDdmMDg1NTFhYzVhNTI1My4uZGU4MzBlOTIzNTJlNWQ1 ZmZlZjBjMDYzMDNjNTNiNTE4YTg4Y2M5YiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0xMC0xOCAgRXZh biBNYXJ0aW4gIDxldmFuQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBzcWxpdGU6IHNob3cgZXh0ZW5kZWQgZXJyb3IgY29kZXMg aW4gZXJyb3IgbG9ncworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NDc4NTkKKworICAgICAgICBTUUxpdGUgY2FuIHByb3ZpZGUgImV4dGVuZGVkIiBlcnJv ciBjb2Rlcywgd2hlcmUgZXh0cmEgaW5mb3JtYXRpb24KKyAgICAgICAgYWJvdXQgdGhlIGVycm9y IGlzIHBvdGVudGlhbGx5IGVuY29kZWQgaW4gdGhlIGhpZ2ggYml0cyBvZiB0aGUgZXJyb3IKKyAg ICAgICAgY29kZS4gIEl0J3Mgb25seSBvZmYgYnkgZGVmYXVsdCBpbiBjYXNlIG9sZGVyIGFwcHMg cmVseSBvbiBlcXVhbGl0eQorICAgICAgICB0ZXN0aW5nIG9uIHRoZSBvbGRlciBlcnJvciBjb2Rl cywgd2hpY2ggV2ViS2l0IGRvZXNuJ3QgZG8uCisKKyAgICAgICAgKiBwbGF0Zm9ybS9zcWwvU1FM aXRlRGF0YWJhc2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U1FMaXRlRGF0YWJhc2U6Om9wZW4p OgorICAgICAgICAqIHBsYXRmb3JtL3NxbC9TUUxpdGVTdGF0ZW1lbnQuY3BwOgorICAgICAgICAo V2ViQ29yZTo6U1FMaXRlU3RhdGVtZW50OjpzdGVwKToKKwogMjAxMC0xMC0xOCAgRGFyaW4gQWRs ZXIgIDxkYXJpbkBhcHBsZS5jb20+CiAKICAgICAgICAgKiBkb20vTm9kZS5oOiBGaXggYW5jaWVu dCB0eXBvIGluIGp1c3QtbW9kaWZpZWQgY29tbWVudC4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGxh dGZvcm0vc3FsL1NRTGl0ZURhdGFiYXNlLmNwcCBiL1dlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0 ZURhdGFiYXNlLmNwcAppbmRleCBhZTk2M2JiNzQ4MjEzYWQ2OTlhMWZiNDhmYjhmMzgzOGJmN2Yy N2I1Li43NDUyMTU5ZDcxYTI1NTg2YWI2MTlhMGJmZTMwYzc5Mzk1ZGMwM2U5IDEwMDY0NAotLS0g YS9XZWJDb3JlL3BsYXRmb3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5jcHAKKysrIGIvV2ViQ29yZS9w bGF0Zm9ybS9zcWwvU1FMaXRlRGF0YWJhc2UuY3BwCkBAIC03Myw2ICs3MywxMyBAQCBib29sIFNR TGl0ZURhdGFiYXNlOjpvcGVuKGNvbnN0IFN0cmluZyYgZmlsZW5hbWUsIGJvb2wgZm9yV2ViU1FM RGF0YWJhc2UpCiAgICAgICAgIG1fZGIgPSAwOwogICAgICAgICByZXR1cm4gZmFsc2U7CiAgICAg fQorICAgIG1fbGFzdEVycm9yID0gc3FsaXRlM19leHRlbmRlZF9yZXN1bHRfY29kZXMobV9kYiwg MSk7CisgICAgaWYgKG1fbGFzdEVycm9yICE9IFNRTElURV9PSykgeworICAgICAgICBMT0dfRVJS T1IoIlNRTGl0ZSBkYXRhYmFzZSBlcnJvciB3aGVuIGVuYWJsaW5nIGV4dGVuZGVkIGVycm9ycyAt ICVzIiwgc3FsaXRlM19lcnJtc2cobV9kYikpOworICAgICAgICBzcWxpdGUzX2Nsb3NlKG1fZGIp OworICAgICAgICBtX2RiID0gMDsKKyAgICAgICAgcmV0dXJuIGZhbHNlOworICAgIH0KIAogICAg IGlmIChpc09wZW4oKSkKICAgICAgICAgbV9vcGVuaW5nVGhyZWFkID0gY3VycmVudFRocmVhZCgp OwpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9wbGF0Zm9ybS9zcWwvU1FMaXRlU3RhdGVtZW50LmNwcCBi L1dlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0ZVN0YXRlbWVudC5jcHAKaW5kZXggMDM3ZWM3OWYz MmQzNzAxMzdkZjI0NGJkYjQ2ZjhmMzAzY2IzOTAwYS4uYTY2ZTMzNTAxNTU2MGU3ZWZkMDgyMmM5 Y2YzNGUwM2IyODY1YWFiMCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9wbGF0Zm9ybS9zcWwvU1FMaXRl U3RhdGVtZW50LmNwcAorKysgYi9XZWJDb3JlL3BsYXRmb3JtL3NxbC9TUUxpdGVTdGF0ZW1lbnQu Y3BwCkBAIC0xMDQsOCArMTA0LDggQEAgaW50IFNRTGl0ZVN0YXRlbWVudDo6c3RlcCgpCiAgICAg TE9HKFNRTERhdGFiYXNlLCAiU1FMIC0gc3RlcCAtICVzIiwgbV9xdWVyeS5hc2NpaSgpLmRhdGEo KSk7CiAgICAgaW50IGVycm9yID0gc3FsaXRlM19zdGVwKG1fc3RhdGVtZW50KTsKICAgICBpZiAo ZXJyb3IgIT0gU1FMSVRFX0RPTkUgJiYgZXJyb3IgIT0gU1FMSVRFX1JPVykgewotICAgICAgICBM T0coU1FMRGF0YWJhc2UsICJzcWxpdGUzX3N0ZXAgZmFpbGVkICglaSlcblF1ZXJ5IC0gJXNcbkVy cm9yIC0gJXMiLCAKLSAgICAgICAgICAgIGVycm9yLCBtX3F1ZXJ5LmFzY2lpKCkuZGF0YSgpLCBz cWxpdGUzX2Vycm1zZyhtX2RhdGFiYXNlLnNxbGl0ZTNIYW5kbGUoKSkpOworICAgICAgICBMT0co U1FMRGF0YWJhc2UsICJzcWxpdGUzX3N0ZXAgZmFpbGVkICglaSlcblF1ZXJ5IC0gJXNcbkVycm9y IDB4JXggLSAlcyIsCisgICAgICAgICAgICBlcnJvciwgbV9xdWVyeS5hc2NpaSgpLmRhdGEoKSwg ZXJyb3IsIHNxbGl0ZTNfZXJybXNnKG1fZGF0YWJhc2Uuc3FsaXRlM0hhbmRsZSgpKSk7CiAgICAg fQogCiAgICAgcmV0dXJuIGVycm9yOwo= 71313 2010-10-20 12:02:38 -0700 2010-10-21 05:59:50 -0700 Patch bug-47859-20101020120236.patch text/plain 1706 evan ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg OGVlM2I5YzQ0NjBkM2YxODNkZTc5MjE5YmUyYmNmZTk3MmFiM2E1Yi4uMDRjN2Q0MDZkZWFlMWUz NmQ2NWFiMmM4OWU2ZDU3MjU1MGM3NjYxMiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0xMC0xOCAgRXZh biBNYXJ0aW4gIDxldmFuQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBzcWxpdGU6IHNob3cgZXh0ZW5kZWQgZXJyb3IgY29kZXMg aW4gZXJyb3IgbG9ncworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NDc4NTkKKworICAgICAgICBTUUxpdGUgY2FuIHByb3ZpZGUgImV4dGVuZGVkIiBlcnJv ciBjb2Rlcywgd2hlcmUgZXh0cmEgaW5mb3JtYXRpb24KKyAgICAgICAgYWJvdXQgdGhlIGVycm9y IGlzIHBvdGVudGlhbGx5IGVuY29kZWQgaW4gdGhlIGhpZ2ggYml0cyBvZiB0aGUgZXJyb3IKKyAg ICAgICAgY29kZS4gIEl0J3Mgb25seSBvZmYgYnkgZGVmYXVsdCBpbiBjYXNlIG9sZGVyIGFwcHMg cmVseSBvbiBlcXVhbGl0eQorICAgICAgICB0ZXN0aW5nIG9uIHRoZSBvbGRlciBlcnJvciBjb2Rl cywgd2hpY2ggV2ViS2l0IGRvZXNuJ3QgZG8uCisKKyAgICAgICAgKiBwbGF0Zm9ybS9zcWwvU1FM aXRlRGF0YWJhc2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U1FMaXRlRGF0YWJhc2U6Om9wZW4p OgorICAgICAgICAqIHBsYXRmb3JtL3NxbC9TUUxpdGVTdGF0ZW1lbnQuY3BwOgorICAgICAgICAo V2ViQ29yZTo6U1FMaXRlU3RhdGVtZW50OjpzdGVwKToKKwogMjAxMC0xMC0yMCAgRGF2aWQgSHlh dHQgIDxoeWF0dEBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgU2ltb24gRnJhc2Vy LgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9wbGF0Zm9ybS9zcWwvU1FMaXRlRGF0YWJhc2UuY3BwIGIv V2ViQ29yZS9wbGF0Zm9ybS9zcWwvU1FMaXRlRGF0YWJhc2UuY3BwCmluZGV4IGI5ZTc2Mzk4NGM5 MTkxZWY3NWM4NWVlYWE4YjIzM2Q4ZjM3NjJhZTEuLmZiMjU4ODliZTk2MWExYTliNzdmYzhjOTRi NzYwZGE5NzRhODcwOTEgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvcGxhdGZvcm0vc3FsL1NRTGl0ZURh dGFiYXNlLmNwcAorKysgYi9XZWJDb3JlL3BsYXRmb3JtL3NxbC9TUUxpdGVEYXRhYmFzZS5jcHAK QEAgLTcyLDYgKzcyLDEyIEBAIGJvb2wgU1FMaXRlRGF0YWJhc2U6Om9wZW4oY29uc3QgU3RyaW5n JiBmaWxlbmFtZSwgYm9vbCBmb3JXZWJTUUxEYXRhYmFzZSkKICAgICAgICAgbV9kYiA9IDA7CiAg ICAgICAgIHJldHVybiBmYWxzZTsKICAgICB9CisgICAgaWYgKHNxbGl0ZTNfZXh0ZW5kZWRfcmVz dWx0X2NvZGVzKG1fZGIsIDEpICE9IFNRTElURV9PSykgeworICAgICAgICBMT0dfRVJST1IoIlNR TGl0ZSBkYXRhYmFzZSBlcnJvciB3aGVuIGVuYWJsaW5nIGV4dGVuZGVkIGVycm9ycyAtICVzIiwg c3FsaXRlM19lcnJtc2cobV9kYikpOworICAgICAgICBzcWxpdGUzX2Nsb3NlKG1fZGIpOworICAg ICAgICBtX2RiID0gMDsKKyAgICAgICAgcmV0dXJuIGZhbHNlOworICAgIH0KIAogICAgIGlmIChp c09wZW4oKSkKICAgICAgICAgbV9vcGVuaW5nVGhyZWFkID0gY3VycmVudFRocmVhZCgpOwo=