Bug 103207

Summary:

Be consistent in handling of *Image::frameAtIndex (and related) return values

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

WebCore Misc.

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, dino, d-r, gtk-ews, noam, peter+ews, senorblanco, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Brent Fulgham

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

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 attachment proposed patch, testcase, etc.

Brent Fulgham

Comment 1 2012-11-25 16:49:32 PST

Created attachment 175898 [details] Patch

WebKit Review Bot

Comment 2 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

Peter Beverloo (cr-android ews)

Comment 3 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

Brent Fulgham

Comment 4 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?

Brent Fulgham

Comment 5 2012-11-25 17:08:54 PST

Created attachment 175901 [details] Patch

WebKit Review Bot

Comment 6 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

Brent Fulgham

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

Brent Fulgham

Comment 8 2012-11-28 14:37:19 PST

Created attachment 176581 [details] Patch

kov's GTK+ EWS bot

Comment 9 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

Dave Hyatt

Comment 10 2012-11-29 11:42:17 PST

Comment on attachment 176581 [details] Patch r=me

WebKit Review Bot

Comment 11 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>

WebKit Review Bot

Comment 12 2012-11-29 12:13:50 PST

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.