Bug 115623

Summary: Crash in Image Decoder due to large gifs
Product: WebKit Reporter: gordon <gosun>
Component: ImagesAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: benjamin, bfulgham, cand, cgarcia, charles.wei, kling, laszlo.gombos, ossy, ostap73, rwlbuis, staikos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Unspecified   
Attachments:
Description Flags
The decoded GIF size > 5M, reserve the initialized frame, when the current frame is the last frame
kling: review-, kling: commit-queue-
The decoded GIF size > 5M, reserve the initialized frame
none
The decoded GIF size > 5M, reserve the initialized frame
benjamin: review-, benjamin: commit-queue-
If the decoded GIF size > 5M, preserve the re-initialized frame
none
If the decoded GIF size > 5M, preserve the frame data.
none
If the decoded GIF size > 5M, preserve the frame data.
none
If the decoded GIF size > 5M, preserve the frame data.
benjamin: review-, benjamin: commit-queue-
If the decoded GIF size > 5M, preserve the re-initialized frame beidson: review-

Description gordon 2013-05-05 23:11:02 PDT
It crashes while playing with the browser for a while, with the following back
trace:

#0  0x7a61730c in setRGBA (a=255, b=<optimized out>, g=<optimized out>, r=255,
dest=<optimized out>, this=<optimized out>)
    at
/home/cswei/project/playbook/webkit/Source/WebCore/platform/image-decoders/ImageDecoder.h:197
#1  WebCore::GIFImageDecoder::haveDecodedRow (this=0x7dd6d940,
frameIndex=<optimized out>, rowBuffer=..., width=<optimized out>, rowNumber=0, 
    repeatCount=1, writeTransparentPixels=false)
    at
webkit/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:236

It is because the decoded GIF size > 5M.
Comment 1 gordon 2013-05-05 23:46:14 PDT
Created attachment 200606 [details]
The decoded GIF size > 5M, reserve the initialized frame, when the current frame is the last frame
Comment 2 Andreas Kling 2013-05-06 06:41:55 PDT
Comment on attachment 200606 [details]
The decoded GIF size > 5M, reserve the initialized frame, when the current frame is the last frame

View in context: https://bugs.webkit.org/attachment.cgi?id=200606&action=review

> Source/WebCore/ChangeLog:8
> +        Crash in Image Decoder due to large gifs
> +        https://bugs.webkit.org/show_bug.cgi?id=115623
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/images/animated-large-image-crash.html

You need to explain what the bug is, and why you are fixing it this way.
Comment 3 gordon 2013-05-06 20:40:47 PDT
Created attachment 200867 [details]
The decoded GIF size > 5M, reserve the initialized frame
Comment 4 gordon 2013-05-07 19:12:05 PDT
Created attachment 201016 [details]
The decoded GIF size > 5M, reserve the initialized frame
Comment 5 Benjamin Poulain 2013-05-07 20:33:21 PDT
Comment on attachment 201016 [details]
The decoded GIF size > 5M, reserve the initialized frame

View in context: https://bugs.webkit.org/attachment.cgi?id=201016&action=review

> LayoutTests/ChangeLog:12
> +        If the decoded GIF is larger than 5M,
> +        we clear the the frame buffer except the current and after frame.
> +        However, if the current frame is the last frame of the GIF,
> +        and the first frame has been re-initialized for next time,
> +        then should not clear first frame, it should be reserved.

This does not explain the test.

Especially since it will decode either only the first frame or no frame at all.

> LayoutTests/ChangeLog:16
> +        * fast/images/resources/gif-large.gif: Added.

Do you own the rights on this?
Better create a new test image yourself.

> Source/WebCore/ChangeLog:13
> +        If the decoded GIF is larger than 5M,
> +        we clear the the frame buffer except the current and after frame.
> +        However, if the current frame is the last frame of the GIF,
> +        and the first frame has been re-initialized for next time,
> +        then should not clear first frame, it should be reserved.
> +        Test: fast/images/animated-large-image-crash.html

This does not explain the reason of the crash.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:270
> +
> +    size_t currentFrame() const
> +    {
> +        return m_currentDecodingFrame;
> +    }
> +

Why do you suddenly expose this publicly?
Comment 6 gordon 2013-05-08 03:34:54 PDT
Created attachment 201051 [details]
If the decoded GIF size > 5M, preserve the re-initialized frame

Thank you for your comments。
> + Why do you suddenly expose this publicly?
I use it to calculate how many frames have been re-initialized, so it should be public.
Comment 7 Rob Buis 2013-05-09 08:27:17 PDT
Comment on attachment 201051 [details]
If the decoded GIF size > 5M, preserve the re-initialized frame

View in context: https://bugs.webkit.org/attachment.cgi?id=201051&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=115623

Is there a PR? Can you try to get an internal review first?
Comment 8 Rob Buis 2013-05-09 08:29:01 PDT
(In reply to comment #7)
> (From update of attachment 201051 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201051&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=115623
> 
> Is there a PR? Can you try to get an internal review first?

Nm, I saw that Benjamin already reviewed it, so you still have to address his issues. I'd also still like to know if there is a PR since that is valuable information if we look at he landed commit a few months later.
Comment 9 gordon 2013-05-09 19:56:42 PDT
Thank you.
PR:328689
Comment 10 gordon 2013-05-09 20:53:46 PDT
Created attachment 201317 [details]
If the decoded GIF size > 5M, preserve the frame data.

Crash in Image Decoder on Master_41 due to gifs.
Comment 11 gordon 2013-05-10 03:57:45 PDT
Created attachment 201331 [details]
If the decoded GIF size > 5M, preserve the frame data.

Crash in Image Decoder on Master_41 due to gifs.
Comment 12 gordon 2013-05-10 03:58:59 PDT
Created attachment 201332 [details]
If the decoded GIF size > 5M, preserve the frame data.

Crash in Image Decoder on Master_41 due to gifs.
Comment 13 Benjamin Poulain 2013-05-10 23:48:32 PDT
Comment on attachment 201332 [details]
If the decoded GIF size > 5M, preserve the frame data.

You should put back the test, this should be tested.
My concern about your previous test was just about the rights. You can just create your own animated gif to solve that.
Comment 14 gordon 2013-05-11 21:41:36 PDT
Sorry, I can't upload this test. Because it will block other build, such as QT.

This patch just fix the BlackBerry, because the previous patch has been "r-"
Comment 15 gordon 2013-05-12 19:14:46 PDT
Created attachment 201519 [details]
If the decoded GIF size > 5M, preserve the re-initialized frame

> + Why do you suddenly expose this publicly? 
I use it to calculate how many frames have been re-initialized, so it should be public.
Comment 16 Charles Wei 2013-05-13 17:42:17 PDT
+Benjamin, Brent to raise the awareness for review.
Comment 17 Alexey Proskuryakov 2013-10-30 09:31:51 PDT
(In reply to comment #14)
> Sorry, I can't upload this test. Because it will block other build, such as QT.

A test can be skipped on other ports by adding an entry to TestExpectations file.
Comment 18 cand 2014-06-13 09:36:32 PDT
I confirm this bug on r169702, on Google's homepage no less!
I also confirm that gosun's last patch (I only applied the gif/* parts, as the assert change in ImageDecoder would conflict with a cairo patch) fixes it.

This is just sad that a serious bug like this can stay open for over a year.
Comment 19 Brady Eidson 2016-05-16 22:43:00 PDT
Comment on attachment 201519 [details]
If the decoded GIF size > 5M, preserve the re-initialized frame

Patch no longer applies to trunk.
If this is still an issue, needs a new patch.