Fix handling of broken GIFs with weird frame sizes

Source/platform/image-decoders/gif/GIFImageReader.cpp

Index: Source/platform/image-decoders/gif/GIFImageReader.cpp
diff --git a/Source/platform/image-decoders/gif/GIFImageReader.cpp b/Source/platform/image-decoders/gif/GIFImageReader.cpp
index 42db80edb56dab57fb92fcea2f215a966c84b03b..a88ef58bbfd729168169a4fa3d3e3a121b67e852 100644
--- a/Source/platform/image-decoders/gif/GIFImageReader.cpp
+++ b/Source/platform/image-decoders/gif/GIFImageReader.cpp
@@ -437,14 +437,10 @@ bool GIFImageReader::parseData(size_t dataPosition, size_t len, GIFImageDecoder:
             // This is the height and width of the "screen" or frame into which images are rendered. The
             // individual images can be smaller than the screen size and located with an origin anywhere
             // within the screen.
+            // Note that size is final only when the first frame is encountered.
             m_screenWidth = GETINT16(currentComponent);
             m_screenHeight = GETINT16(currentComponent + 2);
 
-            // CALLBACK: Inform the decoderplugin of our size.
-            // Note: A subsequent frame might have dimensions larger than the "screen" dimensions.
-            if (m_client && !m_client->setSize(m_screenWidth, m_screenHeight))
-                return false;
-
             const size_t globalColorMapColors = 2 << (currentComponent[4] & 0x07);
 
             if ((currentComponent[4] & 0x80) && globalColorMapColors > 0) { /* global map */
@@ -632,29 +628,18 @@ bool GIFImageReader::parseData(size_t dataPosition, size_t len, GIFImageDecoder:
             width  = GETINT16(currentComponent + 4);
             height = GETINT16(currentComponent + 6);
 
-            /* Work around broken GIF files where the logical screen
-             * size has weird width or height.  We assume that GIF87a
-             * files don't contain animations.
-             */
+            // Work around broken GIF file where the first frame has size greater than the

[Peter Kasting, 2015/01/08 06:56:18]

Nit: files

I also wouldn't mind wrapping the comments to 80 chars.

[Alpha Left Google, 2015/01/12 22:12:49]

Done.

+            // "screen size".

[Peter Kasting, 2015/01/08 06:56:18]

What about when subsequent frames have larger size?  Should we give a
justification for why we don't care about that?  (Is that even possible?  I
don't really have the details of the GIF format paged in, but that's the only
reason I can think of for why we used to have the std::max() calls on old lines
672-3, which you've removed now.)

[Alpha Left Google, 2015/01/12 22:12:49]

I added the std::max() code in 672 and 673 in the last refactoring. Not
realizing that it didn't have any real effect. m_screenWidth and m_screenHeight
is used to set up the canvas. If subsequent frames have larger sizes they don't
change the size of canvas. In fact after notifying the client of the canvas
size, m_screenWidth and m_screenHeight are used only to correct frame size for
corrupted frames.

Once the canvas is set up a subsequent larger frame will be cropped to the
canvas. This is done in GIFImageDecoder. That is why we don't care about this
case here.

             if (currentFrameIsFirstFrame()
-                && ((m_screenHeight < height) || (m_screenWidth < width) || (m_version == 87))) {
-                m_screenHeight = height;
-                m_screenWidth = width;
-                xOffset = 0;
-                yOffset = 0;
-
-                // CALLBACK: Inform the decoderplugin of our size.
-                if (m_client && !m_client->setSize(m_screenWidth, m_screenHeight))
-                    return false;
+                && (m_screenHeight < yOffset + height || m_screenWidth < xOffset + width)) {
+                m_screenHeight = std::max(m_screenHeight, yOffset + height);
+                m_screenWidth = std::max(m_screenWidth, xOffset + width);

[Peter Kasting, 2015/01/08 06:56:18]

This is a behavior change from the previous code in two ways:

(1) We don't unconditionally do this for GIF87a files.  I'm not clear on what
the consequences of this are.
(2) We now respect the x/y offsets, where we didn't before.  This seems possibly
more correct, at least when e.g. width < m_screenWidth < (width + xOffset),
which wouldn't have triggered a resize before but probably should have.  But I'd
like to understand better why the code reset the offsets to begin with, and/or
whether e.g. Firefox still does this.

[Alpha Left Google, 2015/01/12 22:12:49]

The behavior changes unless the image is GIF87 and it is incorrectly formed such
that first frame has size smaller than the canvas size. I've added the condition
of GIF87 back.
FireFox still resets the offsets to 0 and width/height =
m_screenWidth/m_screenHeight. My guess for why it used to reset to 0 is for
simplicity. But I don't think it was correct.

I verified this code change with a corrupted GIF file, which FireFox could not
display. But with this change Chrome can decode correctly.

             }
 
-            // Work around more broken GIF files that have zero image width or height
-            if (!height || !width) {
-                height = m_screenHeight;
-                width = m_screenWidth;
-                if (!height || !width)
-                    return false;
-            }
+            // Inform the client of the final size.
+            if (!m_sizeFinal && m_client && !m_client->setSize(m_screenWidth, m_screenHeight))

[Peter Kasting, 2015/01/08 06:56:18]

Is it possible to have m_client null here but have it be non-null later?  I hope
not, since in that case I don't think we'll ever inform it of the right size.

[Alpha Left Google, 2015/01/12 22:12:49]

m_client is always non null. We shouldn't need to check for m_client but that
can be a later cleanup.

+                return false;
+            m_sizeFinal = true;
 
             if (query == GIFImageDecoder::GIFSizeQuery) {
                 // The decoder needs to stop. Hand back the number of bytes we consumed from
@@ -668,9 +653,15 @@ bool GIFImageReader::parseData(size_t dataPosition, size_t len, GIFImageDecoder:
             GIFFrameContext* currentFrame = m_frames.last().get();
 
             currentFrame->setHeaderDefined();
+
+            // Work around more broken GIF files that have zero image width or height.
+            if (!height || !width) {
+                height = m_screenHeight;
+                width = m_screenWidth;
+                if (!height || !width)
+                    return false;
+            }
             currentFrame->setRect(xOffset, yOffset, width, height);
-            m_screenWidth = std::max(m_screenWidth, width);
-            m_screenHeight = std::max(m_screenHeight, height);
             currentFrame->setInterlaced(currentComponent[8] & 0x40);
 
             // Overlaying interlaced, transparent GIFs over