Bug 39786 – Public GIF decoder fails to mark some images as "done"

• Component: Images
• Status: RESOLVED FIXED

[Peter Kasting]

The open-source GIF decoder has a bug that results in it never marking some GIFs as "done".  The problematic code is effectively:

for (...; bytes remaining > bytes to consume in current state; ...) {
    switch (state) {
    ...
    case x:
        if (consuming bytes led to a finished GIF)
            state = gif_done;
            break;
    ...
    case gif_done:
        clientptr->gifComplete();
        return true;  // "success"
    }
}
return false;  // "need more bytes to continue"

The reason this is a problem is that when we switch the state to gif_done, we don't reset the "bytes to consume" value to 0.  So we go to run through the loop again and find that there are no more bytes in the file, but we want to consume some bytes.  Thus we exit the loop and return false, without ever marking the GIF as "done".

The effect of this is that we erroneously mark these GIFs as having failed, and in some cases fail to properly set things like the repeat count.  Try running Chrome 6.0.408.1 on the URL given in this bug's URL field and note that while the GIFs on the page should all animate indefinitely, most don't until you refresh or reload the page.

The fix here is that instead of setting the state directly, we should use a macro that is used to do all the other state changes, which simultaneously sets the new state and the new "bytes to consume" value (which in this case should be 0).

Patch coming shortly.

[Peter Kasting]

Created attachment 57165 [details]
patch v1

This patch fixes both places that failed to use GETN() to change state: one by using the macro, and the other by eliminating it entirely.  (WebKit uses this code differently than Mozilla did, and marking corrupt GIFs as "failed" won't prevent their display.)

[Adam Barth]

Comment on attachment 57165 [details]
patch v1

I have no idea what this does, but I believe you.  It's too bad we don't have more than one image expert.

[Adam Barth]

Maybe Hyatt has some idea who might know this code better?

[Peter Kasting]

(In reply to comment #3)
> Maybe Hyatt has some idea who might know this code better?

Sadly, I think hyatt and I are the only people to have worked on this, and it was years ago for him.

I probably ought to just gut GIFImageReader.cpp someday and rewrite it from scratch.  It's been a never-ending source of problems.

[Peter Kasting]

Fixed in r60255.