+2006-08-31 Brady Eidson <beidson@apple.com>
+
+ 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 <sam.weinig@gmail.com>
Reviewed by Tim H.
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());
}
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;
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
}
git://git.webkit.org / WebKit-https.git / commitdiff