Bug 103207 - Be consistent in handling of *Image::frameAtIndex (and related) return values
Summary: Be consistent in handling of *Image::frameAtIndex (and related) return values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-25 16:23 PST by Brent Fulgham
Modified: 2012-11-29 12:13 PST (History)
9 users (show)

See Also:

Attachments
Patch (9.99 KB, patch)
2012-11-25 16:49 PST, Brent Fulgham 		no flags Details | Formatted Diff | Diff
Patch (10.40 KB, patch)
2012-11-25 17:08 PST, Brent Fulgham 		no flags Details | Formatted Diff | Diff
Patch (9.71 KB, patch)
2012-11-28 14:37 PST, Brent Fulgham 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2012-11-25 16:23:53 PST 
Under certain conditions, the various image object "frameAtIndex" methods return null.  This means that 'nativeImageForCurrentFrame" can also return null.

This has resulted in a set of patches to ImageSVG (Bug 14531), BitmapImage (Bug 68753), ImageCG (Bug 61684), GraphicsLayerChromium (Bug 98456) and others (Bug 39797) to deal with the null return.

A number of classes check for null return and exit early (ImageWx.cpp, parts of ImageWinCE.cpp, ImageSkia.cpp, ImageCairoWin.cpp, ImageQt.cpp, ImageMac.mm, BitmapImageCG.cpp, BitmapImageCairo.cpp)

However, several others take the return value and use without validating (ImageCGWin.cpp, ImageCairoWin.cpp, parts of ImageWinCE.cpp)

This bug applies the same null checking used elsewhere in the codebase to these missed locations.
Comment 1 Brent Fulgham 2012-11-25 16:49:32 PST 
Created attachment 175898 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-25 16:55:13 PST 
Comment on attachment 175898 [details]
Patch

Attachment 175898 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14982425
Comment 3 Peter Beverloo (cr-android ews) 2012-11-25 16:59:41 PST 
Comment on attachment 175898 [details]
Patch

Attachment 175898 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14989360
Comment 4 Brent Fulgham 2012-11-25 17:05:34 PST 
These kinds of bugs are likely to creep in again.  I wonder if any of C++ method declarations could help in the future?
Comment 5 Brent Fulgham 2012-11-25 17:08:54 PST 
Created attachment 175901 [details]
Patch
Comment 6 WebKit Review Bot 2012-11-25 17:51:57 PST 
Comment on attachment 175901 [details]
Patch

Attachment 175901 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14985439

New failing tests:
svg/W3C-SVG-1.1/animate-elem-52-t.svg
Comment 7 Brent Fulgham 2012-11-25 19:20:39 PST 
I don't think the cr-linux failure is real, based on a review of the test bot output, and the fact that the cr-android test seems to succeed.

Can any Chromium people confirm for me if the patch causes a failure for them?
Comment 8 Brent Fulgham 2012-11-28 14:37:19 PST 
Created attachment 176581 [details]
Patch
Comment 9 kov's GTK+ EWS bot 2012-11-28 14:48:04 PST 
Comment on attachment 176581 [details]
Patch

Attachment 176581 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15023580
Comment 10 Dave Hyatt 2012-11-29 11:42:17 PST 
Comment on attachment 176581 [details]
Patch

r=me
Comment 11 WebKit Review Bot 2012-11-29 12:13:45 PST 
Comment on attachment 176581 [details]
Patch

Clearing flags on attachment: 176581

Committed r136147: <http://trac.webkit.org/changeset/136147>
Comment 12 WebKit Review Bot 2012-11-29 12:13:50 PST 
All reviewed patches have been landed.  Closing bug.