Summary: | [Curl] Not all Cookie database files are deleted on corruption | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Christopher Reid <chris.reid> | ||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, don.olmstead, ews-watchlist, galpeter, Hironori.Fujii, pvollan, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Christopher Reid
2018-05-22 10:40:26 PDT
Created attachment 341007 [details]
Patch
Comment on attachment 341007 [details] Patch Attachment 341007 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7768844 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html Created attachment 341032 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 341007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341007&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:222 > + return true; Should this be a following code? if (resultCode != SQLITE_OK) return false; > Source/WebCore/platform/network/curl/CookieJarDB.cpp:249 > + FileSystem::deleteFile(m_databasePath + "-shm"); Why do you need to do 'deleteFile(m_databasePath + "-shm")' twice? > Source/WebCore/platform/network/curl/CookieJarDB.cpp:510 > + You use checkSQLiteReturnCode in ASSERT macro even though it has a side effect. The side effect is effective only in Debug build. Is this intentional? checkSQLiteReturnCode should be defined as: void CookieJarDB::checkSQLiteReturnCode(int actual) { if (!m_detectedDatabaseCorruption) { switch (actual) { case SQLITE_CORRUPT: case SQLITE_SCHEMA: case SQLITE_FORMAT: case SQLITE_NOTADB: flagDatabaseCorruption(); m_detectedDatabaseCorruption = true; } } } This code should be replaced with: checkSQLiteReturnCode(ret); if (ret != SQLITE_OK && ret != SQLITE_DONE && ret != SQLITE_ROW && !ignoreError) LOG_ERROR("Failed to execute %s error: %s", sql, m_database.lastErrorMsg()); Other ASSERT(checkSQLiteReturnCode(ret, SQLITE_DONE)) should be replaced with a following code: checkSQLiteReturnCode(ret); ASSERT(ret == SQLITE_DONE); Comment on attachment 341007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341007&action=review > Source/WebCore/platform/network/curl/CookieJarDB.cpp:236 > + return false; Inverse the condition for the consistency. if (resultText != "ok") { LOG_ERROR("Cookie database integrity check failed - %s", resultText.ascii().data()); return false; } return true; Created attachment 347252 [details]
Updated patch
(In reply to Fujii Hironori from comment #4) > Comment on attachment 341007 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341007&action=review > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:222 > > + return true; > > Should this be a following code? > if (resultCode != SQLITE_OK) > return false; > Yeah, step also shouldn't even be returning SQLITE_OK. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:249 > > + FileSystem::deleteFile(m_databasePath + "-shm"); > > Why do you need to do 'deleteFile(m_databasePath + "-shm")' twice? > Right, that shouldn't have been added. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:510 > > + > > You use checkSQLiteReturnCode in ASSERT macro even though it has a side > effect. > The side effect is effective only in Debug build. > Is this intentional? > > checkSQLiteReturnCode should be defined as: > > void CookieJarDB::checkSQLiteReturnCode(int actual) > { > if (!m_detectedDatabaseCorruption) { > switch (actual) { > case SQLITE_CORRUPT: > case SQLITE_SCHEMA: > case SQLITE_FORMAT: > case SQLITE_NOTADB: > flagDatabaseCorruption(); > m_detectedDatabaseCorruption = true; > } > } > } > > This code should be replaced with: > > checkSQLiteReturnCode(ret); > if (ret != SQLITE_OK && ret != SQLITE_DONE && ret != SQLITE_ROW && > !ignoreError) > LOG_ERROR("Failed to execute %s error: %s", sql, > m_database.lastErrorMsg()); > > Other ASSERT(checkSQLiteReturnCode(ret, SQLITE_DONE)) should be replaced > with a following code: > > checkSQLiteReturnCode(ret); > ASSERT(ret == SQLITE_DONE); That looks much better. Comment on attachment 347252 [details] Updated patch Clearing flags on attachment: 347252 Committed r234938: <https://trac.webkit.org/changeset/234938> All reviewed patches have been landed. Closing bug. |