WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
67469
[WinCairo] IconDatabase::defaultIcon always fails for non-CAN_THEME_URL_ICON builds.
https://bugs.webkit.org/show_bug.cgi?id=67469
Summary
[WinCairo] IconDatabase::defaultIcon always fails for non-CAN_THEME_URL_ICON ...
David Delaune
Reported
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
Attachments
WinCairo wants CAN_THEME_URL_ICON
(932 bytes, patch)
2011-09-14 15:42 PDT
,
David Delaune
bfulgham
: review-
Details
Formatted Diff
Diff
Changelog added to patch
(1.19 KB, patch)
2011-09-17 13:12 PDT
,
David Delaune
no flags
Details
Formatted Diff
Diff
Updated patch comments
(1.26 KB, patch)
2011-09-20 17:17 PDT
,
David Delaune
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
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.
David Delaune
Comment 2
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
Brady Eidson
Comment 3
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
David Delaune
Comment 4
2011-09-14 15:42:19 PDT
Created
attachment 107412
[details]
WinCairo wants CAN_THEME_URL_ICON WinCairo wants CAN_THEME_URL_ICON
David Delaune
Comment 5
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
Brent Fulgham
Comment 6
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.
David Delaune
Comment 7
2011-09-16 11:34:15 PDT
***
Bug 68261
has been marked as a duplicate of this bug. ***
Brent Fulgham
Comment 8
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).
David Delaune
Comment 9
2011-09-17 13:12:24 PDT
Created
attachment 107772
[details]
Changelog added to patch
Brent Fulgham
Comment 10
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?
David Delaune
Comment 11
2011-09-20 17:17:58 PDT
Created
attachment 108084
[details]
Updated patch comments
Brent Fulgham
Comment 12
2011-09-21 09:57:12 PDT
Comment on
attachment 108084
[details]
Updated patch comments Looks good to me!
WebKit Review Bot
Comment 13
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
>
WebKit Review Bot
Comment 14
2011-09-21 10:09:55 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug