67469 2011-09-02 01:20:34 -0700 [WinCairo] IconDatabase::defaultIcon always fails for non-CAN_THEME_URL_ICON builds. 2011-09-21 10:09:55 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 david.delaune webkit-unassigned beidson bfulgham webkit.review.bot oldest_to_newest 461089 0 david.delaune 2011-09-02 01:20:34 -0700 Hi, I encountered an access violation inside WebKit.dll while implementing favicons. In a nutshell... The loadDefaultIconRecord returns a big-endian TIFF in a static buffer. The ImageDecoder::create is checking the first 14 bytes of the image in order to return the correct ImageDecoder class. The problem is that ImageDecoder::create never even checks for a TIFF header and returns a NULL ImageDecoder* which causes ImageSource::setData to fail. The end-result is an access violation at this line in WebIconDatabase.cpp when attempting to get the default icon. > if (!iconDatabase().defaultIcon(*size)->getHBITMAPOfSize(result, size)) { This call stack shows all of the functions involved: WebKit.dll!WebCore::ImageDecoder::create(const WebCore::SharedBuffer & data={...}, WebCore::ImageSource::AlphaOption alphaOption=AlphaPremultiplied, WebCore::ImageSource::GammaAndColorProfileOption gammaAndColorProfileOption=GammaAndColorProfileApplied) Line 105 C++ WebKit.dll!WebCore::ImageSource::setData(WebCore::SharedBuffer * data=0x7ef30258, bool allDataReceived=true) Line 82 + 0xe bytes C++ WebKit.dll!WebCore::BitmapImage::dataChanged(bool allDataReceived=true) Line 213 C++ WebKit.dll!WebCore::Image::setData(WTF::PassRefPtr<WebCore::SharedBuffer> data={...}, bool allDataReceived=true) Line 77 + 0xe bytes C++ WebKit.dll!WebCore::IconRecord::setImageData(WTF::PassRefPtr<WebCore::SharedBuffer> data={...}) Line 72 + 0x19 bytes C++ WebKit.dll!WebCore::loadDefaultIconRecord(WebCore::IconRecord * defaultIconRecord=0x7eedbc00) Line 375 C++ WebKit.dll!WebCore::IconDatabase::defaultIcon(const WebCore::IntSize & size={...}) Line 386 + 0x9 bytes C++ WebKit.dll!WebIconDatabase::getOrCreateDefaultIconBitmap(tagSIZE * size=0x90050d8c) Line 313 + 0x1b bytes C++ WebKit.dll!WebIconDatabase::defaultIconWithSize(tagSIZE * size=0x0018f6b4, unsigned int * result=0x0018f6c4) Line 194 + 0xc bytes C++ WebKit.dll!WebIconDatabase::iconForURL(wchar_t * url=0x7ee99000, tagSIZE * size=0x0018f6b4, int __formal=1, unsigned int * bitmap=0x0018f6c4) Line 187 + 0x13 bytes C++ [...] Note: The above call stack is before the access violation has occured. The access violation will occur in WebIconDatabase::getOrCreateDefaultIconBitmap due to the NULL Image* pointer. Ports such as QT and GTK might not experience this bug because CAN_THEME_URL_ICON is defined which causes the default icon to be loaded from resource. Best Wishes, -David Delaune 467350 1 beidson 2011-09-14 14:41:36 -0700 You encountered this with the mainline Apple port of WebKit on Windows? You encountered this within Safari for Windows? If the answer to either of these questions is "No", I'd need more info to diagnose what you're running into. 467387 2 david.delaune 2011-09-14 15:11:15 -0700 I am developing with the WinCairo branch but this should have zero effect on the outcome. If you look closely at the code path starting at getOrCreateDefaultIconBitmap you should discover that it is impossible for this function to succeed unless CAN_THEME_URL_ICON is defined. loadDefaultIconRecord() returns a TIFF hard-coded into a static buffer. Following the code path you should find that ImageDecoder::create never checks for a TIFF header and will always result in a NULL ImageDecoder* pointer. Would you like me to modify WinLauncher so that it reveals the access violation? All that you need to do is instantiate an instance of IWebIconDatabase and call iconForURL for any domain without a favicon. Best Wishes, -David Delaune 467403 3 beidson 2011-09-14 15:25:33 -0700 (In reply to comment #2) > I am developing with the WinCairo branch but this should have zero effect on the outcome. If you look closely at the code path starting at getOrCreateDefaultIconBitmap you should discover that it is impossible for this function to succeed unless CAN_THEME_URL_ICON is defined. Then this will only work for CAN_THEME_URL_ICON builds. :) > Would you like me to modify WinLauncher so that it reveals the access violation? All that you need to do is instantiate an instance of IWebIconDatabase and call iconForURL for any domain without a favicon. We don't use WinLauncher extensively, if at all. > > Best Wishes, > -David Delaune 467427 4 107412 david.delaune 2011-09-14 15:42:19 -0700 Created attachment 107412 WinCairo wants CAN_THEME_URL_ICON WinCairo wants CAN_THEME_URL_ICON 467442 5 david.delaune 2011-09-14 15:53:55 -0700 Do you think that perhaps it would be better if I close this as RESOLVED and open a more specific WinCairo bug/feature request? Best Wishes, -David Delaune 468627 6 107412 bfulgham 2011-09-16 11:11:44 -0700 Comment on attachment 107412 WinCairo wants CAN_THEME_URL_ICON This looks good to me. Let's let the EWS system chew on it. 468661 7 david.delaune 2011-09-16 11:34:15 -0700 *** Bug 68261 has been marked as a duplicate of this bug. *** 468684 8 107412 bfulgham 2011-09-16 11:58:06 -0700 Comment on attachment 107412 WinCairo wants CAN_THEME_URL_ICON Overall, the change looks good. However, it is missing a ChangeLog entry that explains the change, who made the change, and provides a link to the bugzilla entry. Please revise the patch to include the ChangeLog (see the http://www.webkit.org/coding/contributing.html page for details). 469094 9 107772 david.delaune 2011-09-17 13:12:24 -0700 Created attachment 107772 Changelog added to patch 469218 10 107772 bfulgham 2011-09-18 20:05:43 -0700 Comment on attachment 107772 Changelog added to patch View in context: https://bugs.webkit.org/attachment.cgi?id=107772&action=review Looks great! > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Please remove this line, or say "No new functionality added in this change." > Source/WebCore/ChangeLog:10 > + * loader/icon/IconDatabase.cpp: Could you just mention here that you are defining CAN_THEME_URL_ICON for the WIN_CAIRO case? 470495 11 108084 david.delaune 2011-09-20 17:17:58 -0700 Created attachment 108084 Updated patch comments 470885 12 108084 bfulgham 2011-09-21 09:57:12 -0700 Comment on attachment 108084 Updated patch comments Looks good to me! 470900 13 108084 webkit.review.bot 2011-09-21 10:09:50 -0700 Comment on attachment 108084 Updated patch comments Clearing flags on attachment: 108084 Committed r95646: <http://trac.webkit.org/changeset/95646> 470901 14 webkit.review.bot 2011-09-21 10:09:55 -0700 All reviewed patches have been landed. Closing bug. 107412 2011-09-14 15:42:19 -0700 2011-09-17 13:12:24 -0700 WinCairo wants CAN_THEME_URL_ICON patch.diff text/plain 932 david.delaune SW5kZXg6IFNvdXJjZS9XZWJDb3JlL2xvYWRlci9pY29uL0ljb25EYXRhYmFzZS5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gU291cmNlL1dlYkNvcmUvbG9hZGVyL2ljb24vSWNvbkRhdGFiYXNlLmNwcAkocmV2 aXNpb24gOTQ0NzYpCisrKyBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvaWNvbi9JY29uRGF0YWJhc2Uu Y3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMSw3ICsyMSw3IEBACiAgKiBQUk9GSVRTOyBPUiBCVVNJ TkVTUyBJTlRFUlJVUFRJT04pIEhPV0VWRVIgQ0FVU0VEIEFORCBPTiBBTlkgVEhFT1JZCiAgKiBP RiBMSUFCSUxJVFksIFdIRVRIRVIgSU4gQ09OVFJBQ1QsIFNUUklDVCBMSUFCSUxJVFksIE9SIFRP UlQKICAqIChJTkNMVURJTkcgTkVHTElHRU5DRSBPUiBPVEhFUldJU0UpIEFSSVNJTkcgSU4gQU5Z IFdBWSBPVVQgT0YgVEhFIFVTRQotICogT0YgVEhJUyBTT0ZUV0FSRSwgRVZFTiBJRiBBRFZJU0VE IE9GIFRIRSBQT1NTSUJJTElUWSBPRiBTVUNIIERBTUFHRS4gCisgKiBPRiBUSElTIFNPRlRXQVJF LCBFVkVOIElGIEFEVklTRUQgT0YgVEhFIFBPU1NJQklMSVRZIE9GIFNVQ0ggREFNQUdFLgogICov CiAKICNpbmNsdWRlICJjb25maWcuaCIKQEAgLTUxLDcgKzUxLDcgQEAKICNkZWZpbmUgSVNfSUNP Tl9TWU5DX1RIUkVBRCgpIChtX3N5bmNUaHJlYWQgPT0gY3VycmVudFRocmVhZCgpKQogI2RlZmlu ZSBBU1NFUlRfSUNPTl9TWU5DX1RIUkVBRCgpIEFTU0VSVChJU19JQ09OX1NZTkNfVEhSRUFEKCkp CiAKLSNpZiBQTEFURk9STShRVCkgfHwgUExBVEZPUk0oR1RLKQorI2lmIFBMQVRGT1JNKFFUKSB8 fCBQTEFURk9STShHVEspIHx8IFBMQVRGT1JNKFdJTl9DQUlSTykKICNkZWZpbmUgQ0FOX1RIRU1F X1VSTF9JQ09OCiAjZW5kaWYKIAo= 107772 2011-09-17 13:12:24 -0700 2011-09-20 17:17:58 -0700 Changelog added to patch patch.diff text/plain 1222 david.delaune SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk1MzgzKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTEtMDktMTcgIERhdmlkIERl bGF1bmUgIDxkYXZpZC5kZWxhdW5lQGdvb2dsZW1haWwuY29tPgorCisgICAgICAgIFtXaW5DYWly b10gSWNvbkRhdGFiYXNlOjpkZWZhdWx0SWNvbiBhbHdheXMgZmFpbHMgZm9yIG5vbi1DQU5fVEhF TUVfVVJMX0lDT04gYnVpbGRzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9Njc0NjkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBObyBuZXcgdGVzdHMuIChPT1BTISkKKworICAgICAgICAqIGxvYWRlci9pY29u L0ljb25EYXRhYmFzZS5jcHA6CisKIDIwMTEtMDktMTcgIElseWEgVGlraG9ub3Zza3kgIDxsb2lz bG9AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IGZpbGUgb3BlbiBkaWFs b2cgYXBwZWFycyB3aGVuIHVzZXIgY2xpY2tzIG9uIHRoZSB0aW1lbGluZSBiYXIgaW4gdGltZWxp bmUgcGFuZWwuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvaWNvbi9JY29uRGF0YWJhc2Uu Y3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2xvYWRlci9pY29uL0ljb25EYXRhYmFz ZS5jcHAJKHJldmlzaW9uIDk0NDc2KQorKysgU291cmNlL1dlYkNvcmUvbG9hZGVyL2ljb24vSWNv bkRhdGFiYXNlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTEsNyArNTEsNyBAQAogI2RlZmluZSBJ U19JQ09OX1NZTkNfVEhSRUFEKCkgKG1fc3luY1RocmVhZCA9PSBjdXJyZW50VGhyZWFkKCkpCiAj ZGVmaW5lIEFTU0VSVF9JQ09OX1NZTkNfVEhSRUFEKCkgQVNTRVJUKElTX0lDT05fU1lOQ19USFJF QUQoKSkKIAotI2lmIFBMQVRGT1JNKFFUKSB8fCBQTEFURk9STShHVEspCisjaWYgUExBVEZPUk0o UVQpIHx8IFBMQVRGT1JNKEdUSykgfHwgUExBVEZPUk0oV0lOX0NBSVJPKQogI2RlZmluZSBDQU5f VEhFTUVfVVJMX0lDT04KICNlbmRpZgogCg== 108084 2011-09-20 17:17:58 -0700 2011-09-21 10:09:50 -0700 Updated patch comments patch.diff text/plain 1289 david.delaune SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk1MzgzKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTEtMDktMTcgIERhdmlkIERl bGF1bmUgIDxkYXZpZC5kZWxhdW5lQGdvb2dsZW1haWwuY29tPgorCisgICAgICAgIFtXaW5DYWly b10gSWNvbkRhdGFiYXNlOjpkZWZhdWx0SWNvbiBhbHdheXMgZmFpbHMgZm9yIG5vbi1DQU5fVEhF TUVfVVJMX0lDT04gYnVpbGRzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9Njc0NjkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBObyBuZXcgZnVuY3Rpb25hbGl0eSBhZGRlZCBpbiB0aGlzIGNoYW5nZS4KKwor ICAgICAgICAqIGxvYWRlci9pY29uL0ljb25EYXRhYmFzZS5jcHA6IGRlZmluaW5nIENBTl9USEVN RV9VUkxfSUNPTiBmb3IgdGhlIFdJTl9DQUlSTworCiAyMDExLTA5LTE3ICBJbHlhIFRpa2hvbm92 c2t5ICA8bG9pc2xvQGNocm9taXVtLm9yZz4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBmaWxl IG9wZW4gZGlhbG9nIGFwcGVhcnMgd2hlbiB1c2VyIGNsaWNrcyBvbiB0aGUgdGltZWxpbmUgYmFy IGluIHRpbWVsaW5lIHBhbmVsLgpJbmRleDogU291cmNlL1dlYkNvcmUvbG9hZGVyL2ljb24vSWNv bkRhdGFiYXNlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvaWNvbi9J Y29uRGF0YWJhc2UuY3BwCShyZXZpc2lvbiA5NDQ3NikKKysrIFNvdXJjZS9XZWJDb3JlL2xvYWRl ci9pY29uL0ljb25EYXRhYmFzZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUxLDcgKzUxLDcgQEAK ICNkZWZpbmUgSVNfSUNPTl9TWU5DX1RIUkVBRCgpIChtX3N5bmNUaHJlYWQgPT0gY3VycmVudFRo cmVhZCgpKQogI2RlZmluZSBBU1NFUlRfSUNPTl9TWU5DX1RIUkVBRCgpIEFTU0VSVChJU19JQ09O X1NZTkNfVEhSRUFEKCkpCiAKLSNpZiBQTEFURk9STShRVCkgfHwgUExBVEZPUk0oR1RLKQorI2lm IFBMQVRGT1JNKFFUKSB8fCBQTEFURk9STShHVEspIHx8IFBMQVRGT1JNKFdJTl9DQUlSTykKICNk ZWZpbmUgQ0FOX1RIRU1FX1VSTF9JQ09OCiAjZW5kaWYKIAo=