Reviewed by Hyatt
authorbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Aug 2006 17:38:19 +0000 (17:38 +0000)
committerbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Aug 2006 17:38:19 +0000 (17:38 +0000)
        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
WebCore/loader/icon/IconDataCache.cpp
WebCore/loader/icon/IconDatabase.cpp
WebCore/loader/icon/SQLDatabase.cpp
WebCore/loader/icon/SQLDatabase.h

index 32a8157052ff01fef325399164f64b1322a484c2..4c2f7de64c145b2d3f2d8943910aa40ec4cd1257 100644 (file)
@@ -1,3 +1,24 @@
+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.
index f8e79c89b2988f8bab3e19a2ea8fbebccd56fb96..433914583e960244f9dc2e3bfa8fb67d5fb75d9b 100644 (file)
@@ -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());
 }
 
index 4dac0185953da22890e67c1e7b21c179adb7bcf7..72bd150b9f9244cbb9e7023ad45f33f3ac1d7d6b 100644 (file)
@@ -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
 }
 
index 4b595a76f3bebd8acc3ba150b32163db89137833..8c62eff05a5450e42a89792969c4b38c7fe38548 100644 (file)
@@ -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
 
 
index 7744d6fedd1faa1d3830d690806958976e83bf2b..690d92bbf169499fd23d9b0d362148616ee078af 100644 (file)
@@ -54,6 +54,7 @@ public:
     void runVacuumCommand();
     
     int64_t lastInsertRowID();
+    int lastChanges();
 
     void setBusyTimeout(int ms);
     void setBusyHandler(int(*)(void*, int));