Bug 102689 – [WinCairo] Crash when requesting favicon.

• Component: WebKit API
• Status: RESOLVED FIXED

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.

Created attachment 174980 [details]
Patch

[Brent Fulgham]

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.

Created attachment 175819 [details]
Patch

Created attachment 175822 [details]
Patch

[Build Bot]

Comment on attachment 175822 [details]
Patch

Attachment 175822 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14964826

[Brent Fulgham]

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?

[Brent Fulgham]

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.

[Brent Fulgham]

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.

Created attachment 176017 [details]
Patch

(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.

[Build Bot]

Comment on attachment 176017 [details]
Patch

Attachment 176017 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14988667

[Brent Fulgham]

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
> [phone]  [e-mail]  <[e-mail]>

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
> [phone]  [e-mail]  <[e-mail]>

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
> [phone]  [e-mail]  <[e-mail]>

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.

Created attachment 176223 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

(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.

Created attachment 176451 [details]
Patch

(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.

[Brent Fulgham]

Comment on attachment 176451 [details]
Patch

Looks great!  Thanks for creating this new test.

[Brent Fulgham]

Committed r136056: <http://trac.webkit.org/changeset/136056>

[Roger Fong]

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

[Roger Fong]

(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

(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 :)

[Brent Fulgham]

(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?