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
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
Comment on attachment 175822 [details] Patch Attachment 175822 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14964826
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 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 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.
Comment on attachment 176017 [details] Patch Attachment 176017 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14988667
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.
Created attachment 176223 [details] Patch
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.
(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.
Comment on attachment 176451 [details] Patch Looks great! Thanks for creating this new test.
Committed r136056: <http://trac.webkit.org/changeset/136056>
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
(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 :)
(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?