From f397dd7ad27b548ae53fb7677dd8907d117d300f Mon Sep 17 00:00:00 2001 From: beidson Date: Thu, 31 Aug 2006 17:38:19 +0000 Subject: [PATCH] Reviewed by Hyatt Fixed an error where an Icon's IconID could change without the change being reflected in the PageURL table, causing icons to be pruned before their time and pages to lose their icons. This is because I misunderstood how SQLite handles the "ON CONFLICT REPLACE" condition, which is to delete the row and re-insert instead of perform an update. Also added an assertion to make sure this doesn't happen again. * loader/icon/IconDataCache.cpp: (WebCore::IconDataCache::writeToDatabase): Instead of one INSERT relying on SQLites conflict handling, broke this into an UPDATE attempt followed by the initial INSERT * loader/icon/IconDatabase.cpp: (WebCore::IconDatabase::createDatabaseTables): Slight tweak to the database schema to prevent this from happening in the future. Note this change will not cause incompatibility with the current schema, therefore I didn't update the official database version number (WebCore::IconDatabase::syncDatabase): Added an ASSERT to look for this condition in the future * loader/icon/SQLDatabase.cpp: (WebCore::SQLDatabase::lastChanges): Added this SQLite accessor to see if an UPDATE command actually changed a row * loader/icon/SQLDatabase.h: Ditto git-svn-id: https://svn.webkit.org/repository/webkit/trunk@16147 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- WebCore/ChangeLog | 21 ++++++++++++++ WebCore/loader/icon/IconDataCache.cpp | 42 ++++++++++++++++++++------- WebCore/loader/icon/IconDatabase.cpp | 6 +++- WebCore/loader/icon/SQLDatabase.cpp | 7 +++++ WebCore/loader/icon/SQLDatabase.h | 1 + 5 files changed, 66 insertions(+), 11 deletions(-) diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog index 32a8157052ff..4c2f7de64c14 100644 --- a/WebCore/ChangeLog +++ b/WebCore/ChangeLog @@ -1,3 +1,24 @@ +2006-08-31 Brady Eidson + + Reviewed by Hyatt + + Fixed an error where an Icon's IconID could change without the change being reflected in the PageURL table, + causing icons to be pruned before their time and pages to lose their icons. This is because I misunderstood + how SQLite handles the "ON CONFLICT REPLACE" condition, which is to delete the row and re-insert instead of + perform an update. Also added an assertion to make sure this doesn't happen again. + + * loader/icon/IconDataCache.cpp: + (WebCore::IconDataCache::writeToDatabase): Instead of one INSERT relying on SQLites conflict handling, broke + this into an UPDATE attempt followed by the initial INSERT + * loader/icon/IconDatabase.cpp: + (WebCore::IconDatabase::createDatabaseTables): Slight tweak to the database schema to prevent this from happening + in the future. Note this change will not cause incompatibility with the current schema, therefore I didn't update + the official database version number + (WebCore::IconDatabase::syncDatabase): Added an ASSERT to look for this condition in the future + * loader/icon/SQLDatabase.cpp: + (WebCore::SQLDatabase::lastChanges): Added this SQLite accessor to see if an UPDATE command actually changed a row + * loader/icon/SQLDatabase.h: Ditto + 2006-08-31 Sam Weinig Reviewed by Tim H. diff --git a/WebCore/loader/icon/IconDataCache.cpp b/WebCore/loader/icon/IconDataCache.cpp index f8e79c89b298..433914583e96 100644 --- a/WebCore/loader/icon/IconDataCache.cpp +++ b/WebCore/loader/icon/IconDataCache.cpp @@ -100,23 +100,45 @@ void IconDataCache::writeToDatabase(SQLDatabase& db) if (m_iconURL.isEmpty()) return; - // First we create and prepare the SQLStatement - String escapedIconURL = escapeSQLString(m_iconURL); - // The following statement works no matter what in version 5 of the DB schema because we have the table - // replace any url entry with the new data if it already exists - SQLStatement sql(db, "INSERT INTO Icon (url,stamp,data) VALUES ('" + escapedIconURL + "', ?, ?);"); - sql.prepare(); + // First we'll try an update in case we're already in the DB + SQLStatement updateAttempt(db, "UPDATE Icon SET stamp = ?, data = ? WHERE url = ?;"); + updateAttempt.prepare(); + + // Bind the url and timestamp + updateAttempt.bindInt64(1, m_stamp); + updateAttempt.bindText16(3, m_iconURL); + + // If we *have* image data, bind it to this statement - Otherwise the DB will get "null" for the blob data, + // signifying that this icon doesn't have any data + if (m_image && !m_image->dataBuffer().isEmpty()) + updateAttempt.bindBlob(2, m_image->dataBuffer().data(), m_image->dataBuffer().size()); + + if (updateAttempt.step() != SQLITE_DONE) { + LOG_ERROR("Failed to update icon data for IconURL %s", m_iconURL.ascii().data()); + return; + } + + // If the UPDATE command actually altered a row, we're done + if (db.lastChanges()) + return; + + + // Otherwise, we've never inserted this Icon URL before, so we'll do so now + updateAttempt.finalize(); + SQLStatement insertStatement(db, "INSERT INTO Icon (url,stamp,data) VALUES (?, ?, ?);"); + insertStatement.prepare(); - // Then we bind the timestamp - sql.bindInt64(1, m_stamp); + // Bind the url and timestamp + insertStatement.bindText16(1, m_iconURL); + insertStatement.bindInt64(2, m_stamp); // Then, if we *have* data, we bind it. Otherwise the DB will get "null" for the blob data, // signifying that this icon doesn't have any data if (m_image && !m_image->dataBuffer().isEmpty()) - sql.bindBlob(2, m_image->dataBuffer().data(), m_image->dataBuffer().size()); + insertStatement.bindBlob(3, m_image->dataBuffer().data(), m_image->dataBuffer().size()); // Finally we step and make sure the step was successful - if (sql.step() != SQLITE_DONE) + if (insertStatement.step() != SQLITE_DONE) LOG_ERROR("Unable to set icon data for IconURL %s", m_iconURL.ascii().data()); } diff --git a/WebCore/loader/icon/IconDatabase.cpp b/WebCore/loader/icon/IconDatabase.cpp index 4dac0185953d..72bd150b9f92 100644 --- a/WebCore/loader/icon/IconDatabase.cpp +++ b/WebCore/loader/icon/IconDatabase.cpp @@ -253,7 +253,7 @@ void IconDatabase::createDatabaseTables(SQLDatabase& db) db.close(); return; } - if (!db.executeCommand("CREATE TABLE Icon (iconID INTEGER PRIMARY KEY AUTOINCREMENT UNIQUE ON CONFLICT REPLACE, url TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE, stamp INTEGER, data BLOB);")) { + if (!db.executeCommand("CREATE TABLE Icon (iconID INTEGER PRIMARY KEY AUTOINCREMENT UNIQUE ON CONFLICT REPLACE, url TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT FAIL, stamp INTEGER, data BLOB);")) { LOG_ERROR("Could not create Icon table in database (%i) - %s", db.lastError(), db.lastErrorMsg()); db.close(); return; @@ -799,6 +799,10 @@ void IconDatabase::syncDatabase() LOG(IconDatabase, "Updating the database took %.4f seconds", timestamp); else LOG(IconDatabase, "Updating the database took %.4f seconds - this is much too long!", timestamp); + + // Check to make sure there are no dangling PageURLs + SQLStatement consistencyCheck(*m_currentDB, "SELECT url FROM PageURL WHERE PageURL.iconID NOT IN (SELECT iconID FROM Icon);"); + ASSERT(!consistencyCheck.returnsAtLeastOneResult()); #endif } diff --git a/WebCore/loader/icon/SQLDatabase.cpp b/WebCore/loader/icon/SQLDatabase.cpp index 4b595a76f3be..8c62eff05a54 100644 --- a/WebCore/loader/icon/SQLDatabase.cpp +++ b/WebCore/loader/icon/SQLDatabase.cpp @@ -149,6 +149,13 @@ int64_t SQLDatabase::lastInsertRowID() return sqlite3_last_insert_rowid(m_db); } +int SQLDatabase::lastChanges() +{ + if (!m_db) + return 0; + return sqlite3_changes(m_db); +} + } // namespace WebCore diff --git a/WebCore/loader/icon/SQLDatabase.h b/WebCore/loader/icon/SQLDatabase.h index 7744d6fedd1f..690d92bbf169 100644 --- a/WebCore/loader/icon/SQLDatabase.h +++ b/WebCore/loader/icon/SQLDatabase.h @@ -54,6 +54,7 @@ public: void runVacuumCommand(); int64_t lastInsertRowID(); + int lastChanges(); void setBusyTimeout(int ms); void setBusyHandler(int(*)(void*, int)); -- 2.36.0