Bug 67469 – [WinCairo] IconDatabase::defaultIcon always fails for non-CAN_THEME_URL_ICON builds.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Delaune]

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

[Brady Eidson]

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.

[David Delaune]

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

[Brady Eidson]

(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

[David Delaune]

Created attachment 107412 [details]
WinCairo wants CAN_THEME_URL_ICON

WinCairo wants CAN_THEME_URL_ICON

[David Delaune]

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

[Brent Fulgham]

Comment on attachment 107412 [details]
WinCairo wants CAN_THEME_URL_ICON

This looks good to me.  Let's let the EWS system chew on it.

[David Delaune]

*** Bug 68261 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

Comment on attachment 107412 [details]
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).

[David Delaune]

Created attachment 107772 [details]
Changelog added to patch

[Brent Fulgham]

Comment on attachment 107772 [details]
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?

[David Delaune]

Created attachment 108084 [details]
Updated patch comments

[Brent Fulgham]

Comment on attachment 108084 [details]
Updated patch comments

Looks good to me!

[WebKit Review Bot]

Comment on attachment 108084 [details]
Updated patch comments

Clearing flags on attachment: 108084

Committed r95646: <http://trac.webkit.org/changeset/95646>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.