Bug 33747 – Handle broken images more correctly in the open-source image decoders

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Peter Kasting]

Bug 32176 fixed a problem where, if an image decoder failed to allocate a buffer for the decoded data, we could later attempt to access the (nonexistent) buffer.  It did this by preventing the access.

The problem with this solution is that the original bug was created when I did the wrong thing while refactoring image decoding code, and in solving my bug, the patch on bug 32176 prevented the display of images which had been partially decoded before failing.

I was wrong to make ImageDecoder::size() ASSERT(!m_failed), since, like all other ImageDecoder functions, the state of the decoder is consistent even when decoding has failed, and it's perfectly valid to ask for the size.  I was also wrong to make RGBA32Buffer::setSize() set the frame status to FrameComplete on allocation failure, because the frame isn't complete, it's unallocated, and thus the existing FrameEmpty state is actually correct.

Fixing these two issues causes bug 32176 to disappear on its own, because ImageSource is already smart enough not to return pointers to frames with status FrameEmpty.  And removing the isSizeAvailable() check added in that bug allows us to (correctly) return non-NULL pointers for cases where the frame is _not_ empty but decoding has failed, thus at least displaying a partial image in some cases.  (If the error occurs after all the pixel data has appeared, then to the user things appear to be completely successful.)

Note that if we were to continue down the path of bug 32176, we'd also need to begin worrying about the other kinds of queries that can be made to an ImageSource when decoding has failed, that might have tripped the ASSERT() in ImageDecoder::size().  Safing these would be annoying, and it's totally unnecessary.

Patch coming momentarily.

[Peter Kasting]

Created attachment 46716 [details]
patch v1

[Shinichiro Hamaji]

It seems the title of this bug wasn't good.

[Peter Kasting]

Why did you remove Adam and Eric, when they were the people on bug 32176?

[Shinichiro Hamaji]

(In reply to comment #3)
> Why did you remove Adam and Eric, when they were the people on bug 32176?

I think you put their email addresses in the subject of this bug. But sorry, I must put them into the CC list when I changed the subject.

[Adam Langley]

LGTM. (Note, I am not a WebKit reviewer. You also need a real review.)

[Eric Seidel (no email)]

Comment on attachment 46716 [details]
patch v1

What does this do, and why?  How do we test it?

[Peter Kasting]

(In reply to comment #6)
> (From update of attachment 46716 [details])
> What does this do, and why?

Comment 0 explained this in detail, was part of it unclear?

> How do we test it?

There is already a test in the tree (added as part of bug 32176) that we can handle invalid images without crashing.  I could perhaps add a pixel test for an invalid but mostly-decodable BMP (with different expected results for Safari and Chromium) although it would be easier to make a small tweak to my pre-existing Chromium-side image decoder tests to check the buffer output in cases where the image partially decoded.  That's about a one-line change.

[Eric Seidel (no email)]

Comment on attachment 46716 [details]
patch v1

I really don't know who should review this simple patch.  I'm confused, even with your explanation in comment 0

[David Levin]

Comment on attachment 46716 [details]
patch v1

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 53348)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Peter Kasting  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Handle broken images more correctly in the open-source image decoders.
> +        https://bugs.webkit.org/show_bug.cgi?id=33747
> +

There should be either be tests in this patch or a note about why there are no tests.

> +        * platform/graphics/ImageSource.cpp:
> +        (WebCore::ImageSource::createFrameAtIndex):

It is nice to add per function level comments when possible. For example, for this function you might say:

Removed a condition added to avoid calling size because it is now always ok to call size here.


> +        * platform/image-decoders/ImageDecoder.h:
> +        (WebCore::ImageDecoder::size):
> +        * platform/image-decoders/qt/RGBA32BufferQt.cpp:
> +        (WebCore::RGBA32Buffer::setSize):
> +        * platform/image-decoders/skia/ImageDecoderSkia.cpp:
> +        (WebCore::RGBA32Buffer::setSize):
> +

[Peter Kasting]

Created attachment 47459 [details]
patch v2

I don't think a patch should be r-ed because the reviewer doesn't think the ChangeLog has enough detail (why not r+ and ask me to fix?), but whatever.

[David Levin]

(In reply to comment #10)
> Created an attachment (id=47459) [details]
> patch v2
> 
> I don't think a patch should be r-ed 

It was r- for lack of a test. This is pretty typical and is expected of WebKit reviewers.

[David Levin]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=47459) [details] [details]
> > patch v2
> > 
> > I don't think a patch should be r-ed 
> 
> It was r- for lack of a test. This is pretty typical and is expected of WebKit
> reviewers.

Oh sorry. I missed that the test issue was discussed in the comments of the bug.

[David Levin]

Comment on attachment 47459 [details]
patch v2

Very minor nit: It would be nice to wrap the comments in the ChangeLog (one of the few places where WebKit seems to try to maintain ~80 columns) and add periods.

Thanks!

[Peter Kasting]

Fixed in 53884.  Scrolling through ChangeLog I didn't see any pattern of sticking to 80 columns, but I did add periods on my comments.