Bug 102689 - [WinCairo] Crash when requesting favicon.
Summary: [WinCairo] Crash when requesting favicon.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Unspecified
: P2 Normal
Assignee: Nobody
URL: http://msdn.microsoft.com
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-19 07:35 PST by peavo
Modified: 2022-03-01 03:02 PST (History)
3 users (show)

See Also:


Attachments
Patch (2.10 KB, patch)
2012-11-19 07:39 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (8.85 KB, patch)
2012-11-23 09:33 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (10.23 KB, patch)
2012-11-23 09:50 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (12.79 KB, patch)
2012-11-26 09:26 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (12.99 KB, patch)
2012-11-27 03:36 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (12.95 KB, patch)
2012-11-28 04:00 PST, peavo
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2012-11-19 07:35:08 PST
When requesting a favicon with IWebIconDatabase::iconForURL(), I get a crash on http://msdn.microsoft.com.
According to the debugger, it crashes when dereferencing a NULL pointer at line 98 (r135155) in WebCore/platform/graphics/win/ImageCairoWin.cpp.
Comment 1 peavo 2012-11-19 07:39:22 PST
Created attachment 174980 [details]
Patch
Comment 2 Brent Fulgham 2012-11-19 17:38:50 PST
Comment on attachment 174980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174980&action=review

This is a great fix, but I think it is generally clearer to do an early return. R- to make that small change.

Also, it would be nice if these Davison bugs could be covered by some kind of test.

> Source/WebCore/ChangeLog:5
> +

Is there any way to test this in our current testing infrastructure

> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:98
> +        if (frameAtIndex(i)) {

I think this might be better as an early return.
Comment 3 peavo 2012-11-23 09:33:52 PST
Created attachment 175819 [details]
Patch
Comment 4 peavo 2012-11-23 09:50:47 PST
Created attachment 175822 [details]
Patch
Comment 5 Build Bot 2012-11-23 10:21:35 PST
Comment on attachment 175822 [details]
Patch

Attachment 175822 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14964826
Comment 6 Brent Fulgham 2012-11-23 21:31:07 PST
Comment on attachment 175822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175822&action=review

> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:98
> +        if (!frameAtIndex(i))

Is it expected that a frame might be null? Maybe we have an underlying problem in frame handling that is allowing null images to pass through.

> Source/WebKit2/win/WebKit2CFLite.def:322
>          ?setTracksRepaints@FrameView@WebCore@@QAEX_N@Z

I'm surprised this is needed; And if it's needed for WebKit2CFLite.def, we should have a similar set of requirements (and tests) on the WebKit2.def side.

> Tools/TestWebKitAPI/win/TestWebKitAPI.vcproj:430
> +						RelativePath="..\Tests\WebCore\win\BitmapImage.cpp"

This will build for WinCairo and regular Windows.  I think this is good, but don't you need to expose the symbols added for WebKit2CFLite.def for our WebKit2.def as well?
Comment 7 Brent Fulgham 2012-11-23 21:47:36 PST
Comment on attachment 175822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175822&action=review

I think this looks very good, but is missing coverage for the main Apple Windows port.  I think the WebKit2.def file needs similar changes, and I'd like to see the config.h for the test handle the Apple CG build.

The EWS also indicates a failure; We should try to keep this green if at all possible.

For these reasons, I'm marking it r-, but I think it's just a few small changes to get this ready to land.

>> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:98
>> +        if (!frameAtIndex(i))
> 
> Is it expected that a frame might be null? Maybe we have an underlying problem in frame handling that is allowing null images to pass through.

I just checked the CG implementation, and they perform similar null checks. So I think this is the right approach.

> Tools/TestWebKitAPI/config.h:51
> +#undef WTF_USE_CG

How does this test get run under the CG build?  This seems incomplete.
Comment 8 Brent Fulgham 2012-11-24 15:06:41 PST
Comment on attachment 175822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175822&action=review

>>> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:98
>>> +        if (!frameAtIndex(i))
>> 
>> Is it expected that a frame might be null? Maybe we have an underlying problem in frame handling that is allowing null images to pass through.
> 
> I just checked the CG implementation, and they perform similar null checks. So I think this is the right approach.

This fix was part of Bug 68753, which I have landed and closed.
Comment 9 peavo 2012-11-26 09:26:31 PST
Created attachment 176017 [details]
Patch
Comment 10 peavo 2012-11-26 09:29:43 PST
(In reply to comment #8)
> (From update of attachment 175822 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175822&action=review
> 
> >>> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:98
> >>> +        if (!frameAtIndex(i))
> >> 
> >> Is it expected that a frame might be null? Maybe we have an underlying problem in frame handling that is allowing null images to pass through.
> > 
> > I just checked the CG implementation, and they perform similar null checks. So I think this is the right approach.
> 
> This fix was part of Bug 68753, which I have landed and closed.

I believe both patches are needed. We still need to check the return value of
frameAtIndex() to avoid dereferencing a null pointer.
Comment 11 Build Bot 2012-11-26 09:46:10 PST
Comment on attachment 176017 [details]
Patch

Attachment 176017 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14988667
Comment 12 Brent Fulgham 2012-11-26 10:09:45 PST
Comment on attachment 176017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176017&action=review

Looks very good.  I think you need one more export to satisfy the EWS, and we're ready to go!

> Source/WebCore/ChangeLog:1
> +2012-11-23  peavo@outlook.com  <peavo@outlook.com>

Can you put a full name here instead of just the e-mail twice?

> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:98
> +        if (!frameAtIndex(i))

I would prefer something like:

NativeImageCairo* nativeImage = frameAtIndex(i);
if (!nativeImage)
    continue;
cairo_surface_t* image = nativeImage->surface();

> Source/WebKit2/ChangeLog:1
> +2012-11-23  peavo@outlook.com  <peavo@outlook.com>

Can you put a full name here instead of just the e-mail twice?

> Source/WebKit2/win/WebKit2.def:347
> +        ?nativeImageForCurrentFrame@BitmapImage@WebCore@@UAEPAUCGImage@@XZ

Looks like we need to add:
 ?notSolidColor@BitmapImage@WebCore@@UAE_NXZ

> Source/WebKit2/win/WebKit2CFLite.def:338
> +        ?nativeImageForCurrentFrame@BitmapImage@WebCore@@UAEPAVNativeImageCairo@2@XZ

Looks like we need to add:
 ?notSolidColor@BitmapImage@WebCore@@UAE_NXZ

> Tools/ChangeLog:1
> +2012-11-23  peavo@outlook.com  <peavo@outlook.com>

Can you put a full name here instead of just the e-mail twice?

> Tools/ChangeLog:10
> +        (TestWebKitAPI):

Delete this row.

> Tools/ChangeLog:11
> +        (BitmapImageTest):

Delete this row.

> Tools/TestWebKitAPI/Tests/WebCore/win/BitmapImage.cpp:2
> + * Copyright (C) 2011, 2012 Apple Inc. All rights reserved.

Copyright should be you, or at least add yourself to the next line.
Comment 13 peavo 2012-11-27 03:36:31 PST
Created attachment 176223 [details]
Patch
Comment 14 Brent Fulgham 2012-11-27 09:15:25 PST
Comment on attachment 176223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176223&action=review

Looks good.  Do you have committer rights?

> Tools/TestWebKitAPI/Tests/WebCore/win/BitmapImage.cpp:35
> +// for and image with empty frames (BitmapImage::frameAtIndex(i) return null), WebKit Bug 102689.

This should be "an image"

> Tools/TestWebKitAPI/Tests/WebCore/win/BitmapImage.cpp:54
> +    int* bits = new int[16*16];

Why not just declare this on the stack, rather than do a new/delete?  It's a small buffer.
Comment 15 Brent Fulgham 2012-11-27 09:16:24 PST
(In reply to comment #13)
> Created an attachment (id=176223) [details]
> Patch

Since you didn't update the name information, I'll assume you wish to avoid using a full name.
Comment 16 peavo 2012-11-28 04:00:46 PST
Created attachment 176451 [details]
Patch
Comment 17 peavo 2012-11-28 04:43:52 PST
(In reply to comment #14)
> (From update of attachment 176223 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176223&action=review
> 
> Looks good.  Do you have committer rights?
> 

No, sorry, I don't have committer rights :)

> > Tools/TestWebKitAPI/Tests/WebCore/win/BitmapImage.cpp:35
> > +// for and image with empty frames (BitmapImage::frameAtIndex(i) return null), WebKit Bug 102689.
> 
> This should be "an image"
> 
> > Tools/TestWebKitAPI/Tests/WebCore/win/BitmapImage.cpp:54
> > +    int* bits = new int[16*16];
> 
> Why not just declare this on the stack, rather than do a new/delete?  It's a small buffer.

Submitted updated patch.
Comment 18 Brent Fulgham 2012-11-28 10:48:09 PST
Comment on attachment 176451 [details]
Patch

Looks great!  Thanks for creating this new test.
Comment 19 Brent Fulgham 2012-11-28 14:23:00 PST
Committed r136056: <http://trac.webkit.org/changeset/136056>
Comment 20 Roger Fong 2012-11-28 14:34:58 PST
Hi can you fix the symbols for Windows?

Windows build breakage:
http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/40258

thanks
Comment 21 Roger Fong 2012-11-28 16:02:11 PST
(In reply to comment #20)
> Hi can you fix the symbols for Windows?
> 
> Windows build breakage:
> http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/40258
> 
> thanks

nvm, bots just need  a kick
Comment 22 peavo 2012-11-29 03:48:21 PST
(In reply to comment #18)
> (From update of attachment 176451 [details])
> Looks great!  Thanks for creating this new test.

Thanks for reviewing, and for the constructive feedback :)
Comment 23 Brent Fulgham 2012-11-29 12:23:54 PST
(In reply to comment #21)
> (In reply to comment #20)
> > Hi can you fix the symbols for Windows?
> > 
> > Windows build breakage:
> > http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/40258
> > 
> > thanks
> 
> nvm, bots just need  a kick

It looks like the Windows (XP) build bot has the same issue.  Could you do whatever you did for the other bots?