Fix handling of broken GIFs with weird frame sizes

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

 /* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
  * http://www.mozilla.org/MPL/
  *
  * Software distributed under the License is distributed on an "AS IS" basis,
@@ skipping 419 matching lines @@
             else
                 return false;
             GETN(7, GIFGlobalHeader);
             break;
         }
 
         case GIFGlobalHeader: {
             // 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 */
                 m_globalColorMap.setTablePositionAndSize(dataPosition, globalColorMapColors);
                 GETN(BYTES_PER_COLORMAP_ENTRY * globalColorMapColors, GIFGlobalColormap);
                 break;
             }
 
             GETN(1, GIFImageStart);
             break;
@@ skipping 167 matching lines @@
             unsigned height, width, xOffset, yOffset;
 
             /* Get image offsets, with respect to the screen origin */
             xOffset = GETINT16(currentComponent);
             yOffset = GETINT16(currentComponent + 2);
 
             /* Get image width and height. */
             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
                 // buffer minus 9 (the amount we consumed to read the header).
                 setRemainingBytes(len + 9);
                 GETN(9, GIFImageHeader);
                 return true;
             }
 
             addFrameIfNecessary();
             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
             // existing image data using the Haeberli display hack
             // requires saving the underlying image in order to
             // avoid jaggies at the transparency edges. We are
             // unprepared to deal with that, so don't display such
             // images progressively. Which means only the first
             // frame can be progressively displayed.
             // FIXME: It is possible that a non-transparent frame
@@ skipping 107 matching lines @@
     rowIter = rowBuffer.begin();
     rowsRemaining = m_frameContext->height();
 
     // Clearing the whole suffix table lets us be more tolerant of bad data.
     for (int i = 0; i < clearCode; ++i) {
         suffix[i] = i;
         suffixLength[i] = 1;
     }
     return true;
 }