More tolerant about malformed GIF files

More tolerant about malformed GIF files

GIF image decoder had became more strict since M27. This caused some malformed
GIFs to regress.

This change matches Mozilla's handling of these malformed GIF files
terminating file parsing as if the file were correct. This allows those
malformed GIF files to show.

BUG=242957
R=jamesr@chromium.org, pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151421

[Peter Kasting, 2013-05-29 04:30:17]

LGTM

https://codereview.chromium.org/15914009/diff/2001/Source/WebKit/chromium/tes...
File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):

https://codereview.chromium.org/15914009/diff/2001/Source/WebKit/chromium/tes...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:319: if (count++ == 1) {
Nit: I think "if (++count == 2)" is slightly clearer, and "++count;  if (count
== 2)" is even better; but,

Better than all this would be if we could do the equivalent of find() within the
SharedBuffer to locate the commas and then append blocks of data at once instead
of going character-by-character; but,

Even better would be to just check a known-bad image into the tree and load that
here, and scrap the algorithm entirely.

https://codereview.chromium.org/15914009/diff/2001/Source/core/platform/image...
File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right):

https://codereview.chromium.org/15914009/diff/2001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:487: if
(*currentComponent == ';') { // terminator.
Nit: You can nuke this block entirely and let it be handled by the catchall code
below, if you update the comment below to begin like:

Having handled the extension and image separator tokens above, the only other
valid token is ';', which terminates the file.  In the case of other characters,
the GIF87a spec tells us...

[Alpha Left Google, 2013-05-29 19:38:58]

https://codereview.chromium.org/15914009/diff/2001/Source/WebKit/chromium/tes...
File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):

https://codereview.chromium.org/15914009/diff/2001/Source/WebKit/chromium/tes...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:319: if (count++ == 1) {
On 2013/05/29 04:30:17, Peter Kasting wrote:
> Nit: I think "if (++count == 2)" is slightly clearer, and "++count;  if (count
> == 2)" is even better; but,
> 
> Better than all this would be if we could do the equivalent of find() within
the
> SharedBuffer to locate the commas and then append blocks of data at once
instead
> of going character-by-character; but,
> 
> Even better would be to just check a known-bad image into the tree and load
that
> here, and scrap the algorithm entirely.

Done. SharedBuffer return const char* so a copy is required. I also try to avoid
adding more data files and reuse existing ones.

https://codereview.chromium.org/15914009/diff/2001/Source/core/platform/image...
File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right):

https://codereview.chromium.org/15914009/diff/2001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:487: if
(*currentComponent == ';') { // terminator.
On 2013/05/29 04:30:17, Peter Kasting wrote:
> Nit: You can nuke this block entirely and let it be handled by the catchall
code
> below, if you update the comment below to begin like:
> 
> Having handled the extension and image separator tokens above, the only other
> valid token is ';', which terminates the file.  In the case of other
characters,
> the GIF87a spec tells us...

Done.

[commit-bot: I haz the power, 2013-05-29 19:40:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/15914009/9001

[commit-bot: I haz the power, 2013-05-29 19:55:49]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Alpha Left Google, 2013-05-29 19:58:27]

+jamesr for OWNERS approval in Source/WebKit/chromium

[Peter Kasting, 2013-05-29 20:02:53]

https://codereview.chromium.org/15914009/diff/2001/Source/WebKit/chromium/tes...
File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):

https://codereview.chromium.org/15914009/diff/2001/Source/WebKit/chromium/tes...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:319: if (count++ == 1) {
On 2013/05/29 19:38:58, Alpha wrote:
> I also try to avoid
> adding more data files and reuse existing ones.

Why?  A file to test this could be a few hundred bytes.  Seems much clearer and
more maintainable than code.

After all, we aadd test files to test other bugs all the time.

[Alpha Left Google, 2013-05-29 20:05:32]

On 2013/05/29 20:02:53, Peter Kasting wrote:
>
https://codereview.chromium.org/15914009/diff/2001/Source/WebKit/chromium/tes...
> File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):
> 
>
https://codereview.chromium.org/15914009/diff/2001/Source/WebKit/chromium/tes...
> Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:319: if (count++ == 1) {
> On 2013/05/29 19:38:58, Alpha wrote:
> > I also try to avoid
> > adding more data files and reuse existing ones.
> 
> Why?  A file to test this could be a few hundred bytes.  Seems much clearer
and
> more maintainable than code.
> 
> After all, we aadd test files to test other bugs all the time.

I thought James was against adding more data files (if we can reuse existing
ones)?

[jamesr, 2013-05-29 21:17:21]

For layout tests, I definitely prefer sharing existing files if they exist. 
This is a little different since it's a unit test and because you aren't reusing
the file, you're generating something in the test.  There's already a
"broken.gif" in Source/WebKit/chromium/tests/data/, is that not what you want? 
If not, adding another one would definitely be OK.

On an unrelated note, if you move the test file to live next to the source file
(i.e. at Source/core/platform/image-deciders/gif/GIFImageDecoderTest.cpp) you
won't need separate OWNERS approval any more.  You should probably do that as a
separate CL that only has the move.

[Alpha Left Google, 2013-05-29 21:21:30]

On 2013/05/29 21:17:21, jamesr wrote:
> For layout tests, I definitely prefer sharing existing files if they exist. 
> This is a little different since it's a unit test and because you aren't
reusing
> the file, you're generating something in the test.  There's already a
> "broken.gif" in Source/WebKit/chromium/tests/data/, is that not what you want?

> If not, adding another one would definitely be OK.
> 
> On an unrelated note, if you move the test file to live next to the source
file
> (i.e. at Source/core/platform/image-deciders/gif/GIFImageDecoderTest.cpp) you
> won't need separate OWNERS approval any more.  You should probably do that as
a
> separate CL that only has the move.

Okay, I'll update the test. broken.gif is broken in a different way that doesn't
apply here.

[Alpha Left Google, 2013-05-29 21:32:23]

Added test file and updated test case.

[Peter Kasting, 2013-05-29 21:33:11]

LGTM

[jamesr, 2013-05-29 21:35:33]

lgtm2

[commit-bot: I haz the power, 2013-05-29 21:37:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/15914009/21001

[commit-bot: I haz the power, 2013-05-29 21:37:03]

Can't process patch for file
Source/WebKit/chromium/tests/data/radient-bad-terminator.gif.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[Alpha Left Google, 2013-05-29 22:50:47]

Committed patchset #5 manually as r151421 (presubmit successful).

[Noel Gordon, 2013-05-30 03:23:13]

Are the image unit-tests run under the blink valgrind, tsan, and asan bots, btw?

[jamesr, 2013-05-30 04:40:59]

On 2013/05/30 03:23:13, Noel Gordon (Google) wrote:
> Are the image unit-tests run under the blink valgrind, tsan, and asan bots,
btw?

They're run as part of webkit_unit_tests.  I don't know if any of the
(valgrind/tsan/asan) bots run this target.

[Noel Gordon, 2013-06-13 05:02:35]

They don't run that target; they do run layout tests.  I'm sure some GIF changes
are not testable with a layout test, but this change could be and that'd help us
maintain/gain coverage on those bots.