Bug 67469

Summary: [WinCairo] IconDatabase::defaultIcon always fails for non-CAN_THEME_URL_ICON builds.
Product: WebKit Reporter: David Delaune <david.delaune>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, bfulgham, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
WinCairo wants CAN_THEME_URL_ICON
bfulgham: review-
Changelog added to patch
none
Updated patch comments none

Description David Delaune 2011-09-02 01:20:34 PDT
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
Comment 1 Brady Eidson 2011-09-14 14:41:36 PDT
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.
Comment 2 David Delaune 2011-09-14 15:11:15 PDT
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
Comment 3 Brady Eidson 2011-09-14 15:25:33 PDT
(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
Comment 4 David Delaune 2011-09-14 15:42:19 PDT
Created attachment 107412 [details]
WinCairo wants CAN_THEME_URL_ICON

WinCairo wants CAN_THEME_URL_ICON
Comment 5 David Delaune 2011-09-14 15:53:55 PDT
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
Comment 6 Brent Fulgham 2011-09-16 11:11:44 PDT
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.
Comment 7 David Delaune 2011-09-16 11:34:15 PDT
*** Bug 68261 has been marked as a duplicate of this bug. ***
Comment 8 Brent Fulgham 2011-09-16 11:58:06 PDT
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).
Comment 9 David Delaune 2011-09-17 13:12:24 PDT
Created attachment 107772 [details]
Changelog added to patch
Comment 10 Brent Fulgham 2011-09-18 20:05:43 PDT
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?
Comment 11 David Delaune 2011-09-20 17:17:58 PDT
Created attachment 108084 [details]
Updated patch comments
Comment 12 Brent Fulgham 2011-09-21 09:57:12 PDT
Comment on attachment 108084 [details]
Updated patch comments

Looks good to me!
Comment 13 WebKit Review Bot 2011-09-21 10:09:50 PDT
Comment on attachment 108084 [details]
Updated patch comments

Clearing flags on attachment: 108084

Committed r95646: <http://trac.webkit.org/changeset/95646>
Comment 14 WebKit Review Bot 2011-09-21 10:09:55 PDT
All reviewed patches have been landed.  Closing bug.