Reviewed by Levi.
authorbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Jun 2006 04:45:43 +0000 (04:45 +0000)
committerbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Jun 2006 04:45:43 +0000 (04:45 +0000)
        In addition to a few small details, logging changes, and style cleanups, this is a stop-gap hack
        for a problem in SQLite's blob handling.  Querying for a blob in a void* form is reproducibly
        returning a corrupt buffer.  The temporary solution is to query for the blob as "quoted text" and
        manually convert the return string into a character buffer.

        * icon/IconDatabase.cpp:
        (WebCore::IconDatabase::recreateDatabase):
        (WebCore::IconDatabase::createPrivateTables):

        (WebCore::hexToUnsignedChar): These two functions are the text-to-character-data converters for the time being
        (WebCore::hexStringToVector):

        (WebCore::IconDatabase::imageDataForIconID):
        (WebCore::IconDatabase::imageDataForIconURL):
        (WebCore::IconDatabase::imageDataForPageURL):
        (WebCore::IconDatabase::iconForPageURL):
        (WebCore::IconDatabase::hasIconForIconURL):
        * icon/IconDatabase.h:  changes the icon data accessors to return a Vector<unsigned char> instead of void*

        * icon/SQLStatement.cpp:
        (WebCore::SQLStatement::columnCount):
        (WebCore::SQLStatement::getColumnBlob):
        * icon/SiteIcon.cpp:
        (SiteIcon::getImage):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@15102 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/icon/IconDatabase.cpp
WebCore/icon/IconDatabase.h
WebCore/icon/SQLStatement.cpp
WebCore/icon/SiteIcon.cpp

index 686dc0c62d472750a51fd9e3e589d2705c567400..0d885b487507b7dd83d3a3e11412fbc88802d396 100644 (file)
@@ -1,3 +1,33 @@
+2006-06-29  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Levi.
+
+        In addition to a few small details, logging changes, and style cleanups, this is a stop-gap hack
+        for a problem in SQLite's blob handling.  Querying for a blob in a void* form is reproducibly
+        returning a corrupt buffer.  The temporary solution is to query for the blob as "quoted text" and
+        manually convert the return string into a character buffer.
+
+        * icon/IconDatabase.cpp:
+        (WebCore::IconDatabase::recreateDatabase):
+        (WebCore::IconDatabase::createPrivateTables):
+
+        (WebCore::hexToUnsignedChar): These two functions are the text-to-character-data converters for the time being
+        (WebCore::hexStringToVector):
+
+        (WebCore::IconDatabase::imageDataForIconID):
+        (WebCore::IconDatabase::imageDataForIconURL):
+        (WebCore::IconDatabase::imageDataForPageURL):
+        (WebCore::IconDatabase::iconForPageURL):
+        (WebCore::IconDatabase::hasIconForIconURL):
+        * icon/IconDatabase.h:  changes the icon data accessors to return a Vector<unsigned char> instead of void*
+
+        * icon/SQLStatement.cpp:
+        (WebCore::SQLStatement::columnCount):
+        (WebCore::SQLStatement::getColumnBlob):
+        * icon/SiteIcon.cpp:
+        (SiteIcon::getImage):
+
+
 2006-06-29  Mitz Pettel  <opendarwin.org@mitzpettel.com>
 
         Reviewed by Darin.
index 5de163d54edca74c0a69209d0698ef43124ed857..0bc0431ae78dcd5e7177201174bcbbb511fb7881 100644 (file)
@@ -113,7 +113,7 @@ void IconDatabase::clearDatabase()
 
 void IconDatabase::recreateDatabase()
 {
-    if (!m_db.executeCommand("CREATE TABLE IconDatabaseInfo (key varchar NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,value NOT NULL ON CONFLICT FAIL);")) {
+    if (!m_db.executeCommand("CREATE TABLE IconDatabaseInfo (key TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,value TEXT NOT NULL ON CONFLICT FAIL);")) {
         LOG_ERROR("Could not create IconDatabaseInfo table in icon.db (%i) - %s", m_db.lastError(), m_db.lastErrorMsg());
         m_db.close();
         return;
@@ -123,17 +123,17 @@ void IconDatabase::recreateDatabase()
         m_db.close();
         return;
     }
-    if (!m_db.executeCommand("CREATE TABLE PageURL (url varchar NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,iconID integer NOT NULL ON CONFLICT FAIL);")) {
+    if (!m_db.executeCommand("CREATE TABLE PageURL (url TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,iconID INTEGER NOT NULL ON CONFLICT FAIL);")) {
         LOG_ERROR("Could not create PageURL table in icon.db (%i) - %s", m_db.lastError(), m_db.lastErrorMsg());
         m_db.close();
         return;
     }
-    if (!m_db.executeCommand("CREATE TABLE Icon (iconID INTEGER PRIMARY KEY AUTOINCREMENT, url NOT NULL UNIQUE ON CONFLICT FAIL, expires INTEGER);")) {
+    if (!m_db.executeCommand("CREATE TABLE Icon (iconID INTEGER PRIMARY KEY AUTOINCREMENT, url TEXT NOT NULL UNIQUE ON CONFLICT FAIL, expires INTEGER);")) {
         LOG_ERROR("Could not create Icon table in icon.db (%i) - %s", m_db.lastError(), m_db.lastErrorMsg());
         m_db.close();
         return;
     }
-    if (!m_db.executeCommand("CREATE TABLE IconResource (iconID integer NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,data blob, touch);")) {
+    if (!m_db.executeCommand("CREATE TABLE IconResource (iconID integer NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,data BLOB, touch INTEGER);")) {
         LOG_ERROR("Could not create IconResource table in icon.db (%i) - %s", m_db.lastError(), m_db.lastErrorMsg());
         m_db.close();
         return;
@@ -147,13 +147,13 @@ void IconDatabase::recreateDatabase()
 
 void IconDatabase::createPrivateTables()
 {
-    if (!m_db.executeCommand("CREATE TEMP TABLE TempPageURL (url varchar NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,iconID integer NOT NULL ON CONFLICT FAIL);")) 
+    if (!m_db.executeCommand("CREATE TEMP TABLE TempPageURL (url TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,iconID INTEGER NOT NULL ON CONFLICT FAIL);")) 
         LOG_ERROR("Could not create TempPageURL table in icon.db (%i) - %s", m_db.lastError(), m_db.lastErrorMsg());
 
-    if (!m_db.executeCommand("CREATE TEMP TABLE TempIcon (iconID INTEGER PRIMARY KEY AUTOINCREMENT, url NOT NULL UNIQUE ON CONFLICT FAIL, expires INTEGER);")) 
+    if (!m_db.executeCommand("CREATE TEMP TABLE TempIcon (iconID INTEGER PRIMARY KEY AUTOINCREMENT, url TEXT NOT NULL UNIQUE ON CONFLICT FAIL, expires INTEGER);")) 
         LOG_ERROR("Could not create TempIcon table in icon.db (%i) - %s", m_db.lastError(), m_db.lastErrorMsg());
 
-    if (!m_db.executeCommand("CREATE TEMP TABLE TempIconResource (iconID integer NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,data blob, touch);")) 
+    if (!m_db.executeCommand("CREATE TEMP TABLE TempIconResource (iconID INTERGER NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,data BLOB, touch INTEGER);")) 
         LOG_ERROR("Could not create TempIconResource table in icon.db (%i) - %s", m_db.lastError(), m_db.lastErrorMsg());
 
     if (!m_db.executeCommand("CREATE TEMP TRIGGER temp_create_icon_resource AFTER INSERT ON TempIcon BEGIN INSERT INTO TempIconResource (iconID, data) VALUES (new.iconID, NULL); END;")) 
@@ -170,68 +170,113 @@ void IconDatabase::deletePrivateTables()
         LOG_ERROR("Could not drop TempIconResource table");
 }
 
-const void* IconDatabase::imageDataForIconID(int id, int& size)
+// FIXME - This is a DIRTY, dirty workaround for a problem that we're seeing where certain blobs are having a corrupt buffer
+// returned when we get the result as a const void* blob.  Getting the blob as a textual representation is 100% accurate so this hack
+// does an in place hex-to-character from the textual representation of the icon data.  After I manage to follow up with Adam Swift, the OSX sqlite maintainer,
+// who is too busy to help me until after 7-4-06, this NEEEEEEEEEEEEEEDS to be changed. 
+// *SIGH*
+unsigned char hexToUnsignedChar(UChar h, UChar l)
 {
-    char* query = sqlite3_mprintf("SELECT data FROM IconResource WHERE iconid = %i", id);
-    SQLStatement sql(m_db, String(query));
-    sql.prepare();
-    sqlite3_free(query);
-    if (sql.step() != SQLITE_ROW) {
-        size = 0;
+    unsigned char c;
+    if (h >= '0' && h <= '9')
+        c = h - '0';
+    else if (h >= 'A' && h <= 'F')
+        c = h - 'A' + 10;
+    else {
+        LOG_ERROR("Failed to parse TEXT result from SQL BLOB query");
         return 0;
     }
-    const void* blob = sql.getColumnBlob(0, size);
-    if (!blob) {
-        size = 0;
+    c *= 16;
+    if (l >= '0' && l <= '9')
+        c += l - '0';
+    else if (l >= 'A' && l <= 'F')
+        c += l - 'A' + 10;
+    else {
+        LOG_ERROR("Failed to parse TEXT result from SQL BLOB query");
         return 0;
+    }    
+    return c;
+}
+
+Vector<unsigned char> hexStringToVector(const String& s)
+{
+    LOG(IconDatabase, "hexStringToVector() - s.length is %i", s.length());
+    if (s[0] != 'X' || s[1] != '\'') {
+        LOG(IconDatabase, "hexStringToVector() - string is invalid SQL HEX-string result - %s", s.ascii().data());
+        return Vector<unsigned char>();
+    }
+
+    Vector<unsigned char> result;
+    result.reserveCapacity(s.length() / 2);
+    const UChar* data = s.characters() + 2;
+    int count = 0;
+    while (data[0] != '\'') {
+        if (data[1] == '\'') {
+            LOG_ERROR("Invalid HEX TEXT data for BLOB query");
+            return Vector<unsigned char>();
+        }
+        result.append(hexToUnsignedChar(data[0], data[1]));
+        data++;
+        data++;
+        count++;
     }
-    return blob;
+    
+    LOG(IconDatabase, "Finished atoi() - %i iterations, result size %i", count, result.size());
+    return result;
+}
+
+Vector<unsigned char> IconDatabase::imageDataForIconID(int id)
+{
+    String blob = SQLStatement(m_db, String::sprintf("SELECT data FROM IconResource WHERE iconid = %i", id)).getColumnText(0);
+    if (blob.length() < 1)
+        return Vector<unsigned char>();
+    return hexStringToVector(blob);
 }
 
-const void* IconDatabase::imageDataForIconURL(const String& _iconURL, int& size)
+Vector<unsigned char> IconDatabase::imageDataForIconURL(const String& _iconURL)
 {
     //Escape single quotes for SQL 
     String iconURL = _iconURL;
     iconURL.replace('\'', "''");
-    
-    const void* blob = 0;
+    String blob;
     
     // If private browsing is enabled, we'll check there first as the most up-to-date data for an icon will be there
     if (m_privateBrowsingEnabled) {
-        blob = SQLStatement(m_db, "SELECT TempIconResource.data FROM TempIconResource, TempIcon WHERE TempIcon.url = '" + iconURL + "' AND TempIconResource.iconID = TempIcon.iconID;").getColumnBlob(0,size);
-        if (blob)
-            return blob;
-    }
+        blob = SQLStatement(m_db, "SELECT TempIconResource.data FROM TempIconResource, TempIcon WHERE TempIcon.url = '" + iconURL + "' AND TempIconResource.iconID = TempIcon.iconID;").getColumnText(0);
+        if (blob.length() > 0) {
+            return hexStringToVector(blob);
+        }
+    } 
     
     // It wasn't found there, so lets check the main tables
-    blob = SQLStatement(m_db, "SELECT IconResource.data FROM IconResource, Icon WHERE Icon.url = '" + iconURL + "' AND IconResource.iconID = Icon.iconID;").getColumnBlob(0, size);
-    if (blob
-        return blob;
-    size = 0;
-    return 0;
+    blob = SQLStatement(m_db, "SELECT quote(IconResource.data) FROM IconResource, Icon WHERE Icon.url = '" + iconURL + "' AND IconResource.iconID = Icon.iconID;").getColumnText(0);
+    if (blob.length() < 1)
+        return Vector<unsigned char>();
+    
+    return hexStringToVector(blob);
 }
 
-const void* IconDatabase::imageDataForPageURL(const String& _pageURL, int& size)
+Vector<unsigned char> IconDatabase::imageDataForPageURL(const String& _pageURL)
 {
     //Escape single quotes for SQL 
     String pageURL = _pageURL;
     pageURL.replace('\'', "''");
-    
-    const void* blob = 0;
+    String blob;
     
     // If private browsing is enabled, we'll check there first as the most up-to-date data for an icon will be there
     if (m_privateBrowsingEnabled) {
-        blob = SQLStatement(m_db, "SELECT TempIconResource.data FROM TempIconResource, TempPageURL WHERE TempPageURL.url = '" + pageURL + "' AND TempIconResource.iconID = TempPageURL.iconID;").getColumnBlob(0,size);
-        if (blob)
-            return blob;
-    }
+        blob = SQLStatement(m_db, "SELECT TempIconResource.data FROM TempIconResource, TempPageURL WHERE TempPageURL.url = '" + pageURL + "' AND TempIconResource.iconID = TempPageURL.iconID;").getColumnText(0);
+        if (blob.length() > 0) {
+            return hexStringToVector(blob);
+        }
+    } 
     
     // It wasn't found there, so lets check the main tables
-    blob = SQLStatement(m_db, "SELECT IconResource.data FROM IconResource, PageURL WHERE PageURL.url = '" + pageURL + "' AND IconResource.iconID = PageURL.iconID;").getColumnBlob(0, size);
-    if (blob
-        return blob;
-    size = 0;
-    return 0;
+    blob = SQLStatement(m_db, "SELECT quote(IconResource.data) FROM IconResource, PageURL WHERE PageURL.url = '" + pageURL + "' AND IconResource.iconID = PageURL.iconID;").getColumnText(0);
+    if (blob.length() < 1)
+        return Vector<unsigned char>();
+    
+    return hexStringToVector(blob);
 }
 
 void IconDatabase::setPrivateBrowsingEnabled(bool flag)
@@ -426,7 +471,7 @@ bool IconDatabase::hasIconForIconURL(const String& _url)
     }
     
     String query = "SELECT IconResource.data FROM IconResource, Icon WHERE Icon.url = '" + url + "' AND IconResource.iconID = Icon.iconID;";
-    int size;
+    int size = 0;
     const void* data = SQLStatement(m_db, query).getColumnBlob(0, size);
     return (data && size);
 }
index d6511b609132055cb0d26915e8850cee265aaef0..113d3f61facf36d96bdbf5f6ec9ed58fb3e79e66 100644 (file)
@@ -131,9 +131,9 @@ private:
     // The following three methods follow the sqlite convention for blob data
     // They return a const void* which is a pointer to the data buffer, and store
     // the size of the data buffer in the int& parameter
-    const void* imageDataForIconID(int, int&);
-    const void* imageDataForIconURL(const String&, int&);
-    const void* imageDataForPageURL(const String&, int&);
+    Vector<unsigned char> imageDataForIconID(int);
+    Vector<unsigned char> imageDataForIconURL(const String&);
+    Vector<unsigned char> imageDataForPageURL(const String&);
     
     static IconDatabase* m_sharedInstance;
     static const int DefaultCachedPageCount;
index 8c06d2b8d7b2a70f9e6c87498589e5b33768c502..1f3f08b4d54c84a5c93792f223507bf8fed64185 100644 (file)
@@ -140,7 +140,7 @@ int SQLStatement::bindInt64(int index, int64_t integer)
 int SQLStatement::columnCount()
 {
     if (m_statement)
-        return sqlite3_column_count(m_statement);
+        return sqlite3_data_count(m_statement);
     return 0;
 }
 
@@ -236,11 +236,13 @@ Vector<char> SQLStatement::getColumnBlobAsVector(int col)
 
 const void* SQLStatement::getColumnBlob(int col, int& size)
 {
-    if (!m_statement)
-        if (prepareAndStep() != SQLITE_ROW) {
-            size = 0;
-            return 0;
-        }
+    if (finalize() != SQLITE_OK)
+        LOG(IconDatabase, "Finalize failed");
+    if (prepare() != SQLITE_OK)
+        LOG(IconDatabase, "Prepare failed");
+    if (step() != SQLITE_ROW)
+        {LOG(IconDatabase, "Step wasn't a row");size=0;return 0;}
+
     if (columnCount() <= col) {
         size = 0;
         return 0;
index 1d8e029950137d45bc36d1c6edcfd9008403e12f..722061ff0b0e21f4d64f4fed390e816ec4fa7893 100644 (file)
@@ -52,18 +52,33 @@ Image* SiteIcon::getImage(const IntSize& size)
         return m_image;
     
     if (IconDatabase::m_sharedInstance) {
-        int size;
-        const void* imageData = IconDatabase::m_sharedInstance->imageDataForIconURL(m_iconURL, size);
-        if (!imageData || !size)
+        Vector<unsigned char> imageData = IconDatabase::m_sharedInstance->imageDataForIconURL(m_iconURL);
+        LOG(IconDatabase, "For URL %s, we got a buffer of size %i", m_iconURL.ascii().data(), imageData.size());
+
+        if (!imageData.size())
             return 0;
+
+        String hexdata;
+        int checksum = 0;
+        for (unsigned int i=0; i<imageData.size(); ++i) {
+            checksum += imageData[i];
+            hexdata.append(String::sprintf("%.2hhX", imageData[i]));
+        }
+            
         NativeBytePtr nativeData = 0;
         // FIXME - Any other platform will need their own method to create NativeBytePtr from the void*
 #ifdef __APPLE__
-        nativeData = CFDataCreate(NULL, (const UInt8*)imageData, size);
+        nativeData = CFDataCreate(NULL, imageData.data(), imageData.size());
 #endif
         m_image = new Image();
-        if (m_image->setNativeData(nativeData, true))
+        
+
+        LOG(IconDatabase,"DUMP-\n%s", hexdata.ascii().data());
+        if (m_image->setNativeData(nativeData, true)) {
+            LOG(IconDatabase, "%s\nImage Creation SUCCESSFUL - %i bytes of data with a checksum of %i", m_iconURL.ascii().data(), imageData.size(), checksum);
             return m_image;
+        }
+        LOG(IconDatabase,     "%s\nImage Creation FAILURE    - %i bytes of data with a checksum of %i", m_iconURL.ascii().data(), imageData.size(), checksum);
         delete m_image;
         return m_image = 0;
     }