Bug 35411 – Clean up m_failed usage in open-source image decoders

• Component: Images
• Status: RESOLVED FIXED

[Peter Kasting]

If we move m_failed to be private, and make setFailed() virtual, then subclasses can safely override it to do automatic cleanup on failure.

We can also make setFailed() return false for easy tailcalling.

[Peter Kasting]

Created attachment 49553 [details]
Part 1, v1

I'm saving the actual override bits for followup patches because they can modify object lifetimes, which can cause hairy bugs and thus needs close review.  This patch should have no functional effect.

[WebKit Review Bot]

Attachment 49553 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:299:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 49554 [details]
Part 1, v2

Fix style violation

[WebKit Review Bot]

Attachment 49554 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/314114

[Peter Kasting]

Created attachment 49828 [details]
Part 1, v3

Try to fix Qt compile failure.

[WebKit Review Bot]

Attachment 49828 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/ImageDecoderQt.cpp:174:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 49831 [details]
Part 1, v4

Fix existing style issues in ImageDecoderQt.cpp.

[Holger Freyther]

The Qt bits seems to be fine.

[Adam Barth]

Comment on attachment 49831 [details]
Part 1, v4

Looks reasonable.  Moving code to the other side of setJump / longJump is kind of scary, but it seemed right.

[Peter Kasting]

Comment on attachment 49831 [details]
Part 1, v4

Landed in r55687, clearing flags.

[Peter Kasting]

Created attachment 50736 [details]
Part 2, v1

Cleanup part 2.  This makes all the decoders behave the same w.r.t. setting the failure bit when we reach end-of-data and the image is still incomplete.

[WebKit Review Bot]

Attachment 50736 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:293:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:930:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 50738 [details]
Part 2, v2

This fixes one style violation.  The other is not being fixed because this whole file follows 2-space indent.

[WebKit Review Bot]

Attachment 50738 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:930:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 50738 [details]
Part 2, v2

The ChangeLog is missing too sparse for me to effectively review this.

Ideally there would be per function comments which explain why that function was changed.

For example, I can't tell why the changes in do_lzw were done because it changes the return value in a lot of places. I'm sure this is obvious to you and probably obvious for someone who knows the code well but neither of those is true for me.

Similarly, in GIFImageReader::read, it seems that it may return true in a few cases in which it returned false before like in this code:
    if (!map)
        return clientptr ? clientptr->setFailed() : false;

I don't understand why this is good (from reading the ChangeLog and the code).

Lastly, there is no new layout tests or explanation of why one either isn't needed or can't be done.

[Peter Kasting]

Created attachment 54743 [details]
Part 2, v3

Reposting with a more complete ChangeLog, including per-function comments and a note about layout tests.

[WebKit Review Bot]

Attachment 54743 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:930:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 54743 [details]
Part 2, v3

Please consider addressing the "Should we..." and compacting the ChangeLog slightly through the use of ditto as mentioned below.

Thanks!

 
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 58541)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,37 @@
> [phone]  Peter Kasting  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Make all image decoders set the "failed" bit if an image could not be
> +        completely decoded, but no more data is coming.  The ICO and BMP
> +        decoders already did this.
> +        https://bugs.webkit.org/show_bug.cgi?id=35411
> +
> +        "Failed" does not cause the image to not be displayed, it simply causes
> +        us to not bother to try to decode again if future requests are made, and
> +        for some decoders, lets the decoder clean up some of its temporary
> +        objects.
> +        
> +        No layout tests because this does not change the visible output of decoding in any way.
> +
> +        * platform/image-decoders/gif/GIFImageDecoder.cpp:
> +        (WebCore::GIFImageDecoder::frameComplete): Return whether the frame could be marked as complete.
> +        (WebCore::GIFImageDecoder::decode): read() will mark decode failure itself, so the only additional failure case is when read() needs more data (and thus returns false) and no more is coming.
> +        * platform/image-decoders/gif/GIFImageDecoder.h:
> +        * platform/image-decoders/gif/GIFImageReader.cpp:
> +        (GIFImageReader::do_lzw): Instead of returning true for buffer underrun and false for failure, return false for both and set the failure flag on failure.
> +        (GIFImageReader::read): Instead of returning true for buffer underrun and false for failure, return false for both and set the failure flag on failure.

"Ditto." works instead of repeating the last phrase exactly.

Thanks for the notes. This helps me understand what you are doing better (and I unfortunately kept reading "setFailed()" as "failure()" which was my other source of confusion).



> Index: WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp
> +bool GIFImageDecoder::frameComplete(unsigned frameIndex, unsigned frameDuration, RGBA32Buffer::FrameDisposalMethod disposalMethod)
>  {
>      // Initialize the frame if necessary.  Some GIFs insert do-nothing frames,
>      // in which case we never reach haveDecodedRow() before getting here.
>      RGBA32Buffer& buffer = m_frameBufferCache[frameIndex];
>      if ((buffer.status() == RGBA32Buffer::FrameEmpty) && !initFrameBuffer(frameIndex))
> -        return;
> +        return false; // initFrameBuffer() has already called setFailed().

Should we "assert(failed());" here (before the return and the comment could be right after the assert)?


> Index: WebCore/platform/image-decoders/gif/GIFImageReader.cpp
>          // Even though suffix[] only holds characters through suffix[avail - 1],
>          // allowing code >= avail here lets us be more tolerant of malformed
> @@ -324,7 +326,7 @@ bool GIFImageReader::do_lzw(const unsign
>          code = prefix[code];
>  
>          if (stackp == stack + MAX_BITS)
> -          return false;
> +          return clientptr ? clientptr->setFailed() : false;

I kept reading this as possibly returning two different values but it always returns false.


> +      if (!do_lzw(q))
> +        return false; // If do_lzw() encountered an error, it has already called
> +                      // clientptr->setFailed().


Should we "assert(failed());" here (before the return and the comment could be right after the assert)?

> +        if (clientptr && frame_reader && !clientptr->frameComplete(images_decoded - 1, frame_reader->delay_time, frame_reader->disposal_method))
> +          return false; // frameComplete() has already called
> +                        // clientptr->setFailed().

Ditto.

[Peter Kasting]

(In reply to comment #18)
> > +        (GIFImageReader::do_lzw): Instead of returning true for buffer underrun and false for failure, return false for both and set the failure flag on failure.
> > +        (GIFImageReader::read): Instead of returning true for buffer underrun and false for failure, return false for both and set the failure flag on failure.
> 
> "Ditto." works instead of repeating the last phrase exactly.

OK

> > Index: WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp
> > +bool GIFImageDecoder::frameComplete(unsigned frameIndex, unsigned frameDuration, RGBA32Buffer::FrameDisposalMethod disposalMethod)
> >  {
> >      // Initialize the frame if necessary.  Some GIFs insert do-nothing frames,
> >      // in which case we never reach haveDecodedRow() before getting here.
> >      RGBA32Buffer& buffer = m_frameBufferCache[frameIndex];
> >      if ((buffer.status() == RGBA32Buffer::FrameEmpty) && !initFrameBuffer(frameIndex))
> > -        return;
> > +        return false; // initFrameBuffer() has already called setFailed().
> 
> Should we "assert(failed());" here (before the return and the comment could be
> right after the assert)?

We could, although I'm not sure what it buys us.  It wouldn't make sense to ever change initFrameBuffer()'s return value to mean something else, since it consumes no data, so I can't see any future-proofing issue here.  Plus, if this were somehow able to go wrong, it couldn't cause any problems, we would just fail to early-exit.  So I think an assert() is overkill.

> > +      if (!do_lzw(q))
> > +        return false; // If do_lzw() encountered an error, it has already called
> > +                      // clientptr->setFailed().
> 
> 
> Should we "assert(failed());" here (before the return and the comment could be
> right after the assert)?

We can't; false usually doesn't mean failure here (note the "If" beginning the comment).

> > +        if (clientptr && frame_reader && !clientptr->frameComplete(images_decoded - 1, frame_reader->delay_time, frame_reader->disposal_method))
> > +          return false; // frameComplete() has already called
> > +                        // clientptr->setFailed().
> 
> Ditto.

Same comment as on asserting initFrameBuffer() has setFailed().

[Peter Kasting]

Comment on attachment 54743 [details]
Part 2, v3

Landed in r58589, clearing flags.

[Peter Kasting]

Created attachment 54817 [details]
Part 3, v1

Here is the last piece.  This actually makes use of the ability to override setFailed() to clean up on failure.

[David Levin]

btw, this bug now has 22 comments. It may be hard for folks to tell if the current patch has any comments about it. You may want to consider only doing one patch (with multiple iterations on that patch) per bug, which is considered a best practice by many for this reason. In other words, consider moving "Part 3" into a new bug.


(In reply to comment #19)
> > > +        return false; // initFrameBuffer() has already called setFailed().
> > 
> > Should we "assert(failed());" here (before the return and the comment could be
> > right after the assert)?
> 
> We could, although I'm not sure what it buys us.  It wouldn't make sense to
> ever change initFrameBuffer()'s return value to mean something else, since it
> consumes no data, so I can't see any future-proofing issue here.  Plus, if this
> were somehow able to go wrong, it couldn't cause any problems, we would just
> fail to early-exit.  So I think an assert() is overkill.

This confuses me because there is a significant comment that explains that initFrameBuffer() has already called setFailed. However, a one line assert to valid that comment is overkill?

In my vast and varied experience, I've found asserts tend to validate assumptions better than comments. :)



> > > +      if (!do_lzw(q))
> > > +        return false; // If do_lzw() encountered an error, it has already called
> > > +                      // clientptr->setFailed().
> > 
> > Should we "assert(failed());" here (before the return and the comment could be
> > right after the assert)?
> 
> We can't; false usually doesn't mean failure here (note the "If" beginning the
> comment).

Ok. Makes sense, but now I don't understand why the comment is there because shouldn't a method always call clientptr->setFailed() if it encounters an error (especially if you can't tell by its return value whether it failed or not)?

[Peter Kasting]

(In reply to comment #22)
> btw, this bug now has 22 comments. It may be hard for folks to tell if the
> current patch has any comments about it. You may want to consider only doing
> one patch (with multiple iterations on that patch) per bug, which is considered
> a best practice by many for this reason. In other words, consider moving "Part
> 3" into a new bug.

I don't think it makes sense to file multiple bugs for a single logical issue when the individual pieces of it don't really get anywhere useful on their own.  The individual subcomponents aren't bugs.

> This confuses me because there is a significant comment that explains that
> initFrameBuffer() has already called setFailed.

A few words is "a significant comment"?  I thought it was a fairly trivial comment.  As I explained, if setFailed() hadn't been called, nothing bad would happen here.

> I don't understand why the comment is there because
> shouldn't a method always call clientptr->setFailed() if it encounters an error
> (especially if you can't tell by its return value whether it failed or not)?

No; calling setFailed() may destroy |this| (as it will in this case after landing Part 3), and thus accessing members like |clientptr| could cause a crash.

[Adam Barth]

Comment on attachment 54817 [details]
Part 3, v1

This patch has been up for review for a long time.  Like many patches to the image decoders, Peter is the only one who really understands them, consequently my review here is largely based on my faith in Peter.  I was tempted to give this patch an R- because it explain the problem it's solving and has some oddities (such as m_doNothingOnFailure), but on balance, I'm willing to believe this is an improvement.

Peter, I'm sorry contributing to the image decoders is such a pain.  I don't really have much of a solution to over you though.  I suspect the net result is that you're going to end up working on other things and the decoders will sit around in their current state.  In the end, we'll muddle through.

Thanks for the patch.

WebCore/ChangeLog:13
 +          No layout tests because this does not change the visible output of
What's the consequence of failing to clean up these objects?

WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp: 
 +              close();
What happened to this call to close()?

WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:232
 +      m_doNothingOnFailure = true;
:(

WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:209
 +          return false;
We don't even call the parent class setFailed method in this case?

[Peter Kasting]

(In reply to comment #24)
> WebCore/ChangeLog:13
>  +          No layout tests because this does not change the visible output of
> What's the consequence of failing to clean up these objects?

The way we currently do?  Higher memory usage, although I doubt it would really matter unless a page had a ton of failed images.

> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp: 
>  +              close();
> What happened to this call to close()?

setFailed() now destroys the JPEGImageReader.  The reader calls close() in its destructor.

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:232
>  +      m_doNothingOnFailure = true;
> :(

Yeah, I hate this too.  I couldn't come up with a better solution.  I would love something that sucks less, ugh.

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:209
>  +          return false;
> We don't even call the parent class setFailed method in this case?

There's no need; as soon as we reset m_doNothingOnFailure, we check whether we failed and call setFailed() at that point.

[Peter Kasting]

Fixed in r61619.