Refactor PNGImageDecoder's call to setSize

Refactor PNGImageDecoder's call to setSize

In the existing code, if setSize fails, PNGImageDecoder attempts to
dereference the png_structp owned by m_reader. But setSize calls
setFailed on failure, deleting m_reader in the process.

As it turns out, it is unnecessary to dereference the png_structp in
this case anyway. The intent is to call longjmp in order to stop
libpng's processing, but this block of code is only executed when this
method is called directly by m_reader, so no need to longjmp.

This method, headerAvailable, is called for each frame of a PNG, but
the setup code (e.g. setSize) only needs to be done once for the entire
image. Separate the pieces that only need to be done once from
headerAvailable. This makes it more clear that there is no need to
longjmp; use a boolean return instead. Now if setSize fails, no part
of m_reader will be accessed.

Move the extra size check into a new override of setSize, and move the
color space setup code to its own method.

Add a test image that reports a size that is too big for
ImageDecoder::setSize. Attempting to decode the size should fail.

BUG=702934
Review-Url: https://codereview.chromium.org/2766263002
Cr-Commit-Position: refs/heads/master@{#459074}
Committed: https://chromium.googlesource.com/chromium/src/+/fab1444fe6528ac403acf28945492f001f3d932e

[scroggo_chromium, 2017-03-22 17:00:19]

scroggo@chromium.org changed reviewers:
+ msarett@chromium.org, pkasting@chromium.org

[scroggo_chromium, 2017-03-22 17:00:20]

PTAL

I've cc'ed you both on the bug this fixes (crbug.com/702934), so I think you
should be able to see the extra details.

[scroggo_chromium, 2017-03-22 17:40:10]

The CQ bit was checked by scroggo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 17:40:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 19:23:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 19:23:07]

Dry run: This issue passed the CQ dry run.

[msarett1, 2017-03-22 19:28:06]

lgtm

https://codereview.chromium.org/2766263002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2766263002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:206:
// TODO(msarret): Add GRAY profile support, block CYMK?
nit: I realize that you're just moving this code, but feel free to fix the
spelling on msarett :).

[scroggo_chromium, 2017-03-22 19:37:18]

https://codereview.chromium.org/2766263002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2766263002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:206:
// TODO(msarret): Add GRAY profile support, block CYMK?
On 2017/03/22 19:28:06, msarett1 wrote:
> nit: I realize that you're just moving this code, but feel free to fix the
> spelling on msarett :).

Haha done! Sorry for overlooking.

[Peter Kasting, 2017-03-22 21:59:52]

LGTM

https://codereview.chromium.org/2766263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2766263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:218:
return ImageDecoder::setSize(width, height);
Nit: Or:

  return (width <= maxPNGSize) && (height <= maxPNGSize) &&
         ImageDecoder::setSize(width, height);

[scroggo_chromium, 2017-03-23 12:24:41]

https://codereview.chromium.org/2766263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
(right):

https://codereview.chromium.org/2766263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:218:
return ImageDecoder::setSize(width, height);
On 2017/03/22 21:59:52, Peter Kasting wrote:
> Nit: Or:
> 
>   return (width <= maxPNGSize) && (height <= maxPNGSize) &&
>          ImageDecoder::setSize(width, height);

Done.

[scroggo_chromium, 2017-03-23 12:24:48]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2017-03-23 12:24:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
msarett@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2766263002/#ps40001
(title: "Single return statement")

[commit-bot: I haz the power, 2017-03-23 12:24:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 14:17:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490271888054740,
"parent_rev": "9d21d637803bb3b0e674ca2c060aac9947146fab", "commit_rev":
"fab1444fe6528ac403acf28945492f001f3d932e"}

[commit-bot: I haz the power, 2017-03-23 14:18:03]

Description was changed from

==========
Refactor PNGImageDecoder's call to setSize

In the existing code, if setSize fails, PNGImageDecoder attempts to
dereference the png_structp owned by m_reader. But setSize calls
setFailed on failure, deleting m_reader in the process.

As it turns out, it is unnecessary to dereference the png_structp in
this case anyway. The intent is to call longjmp in order to stop
libpng's processing, but this block of code is only executed when this
method is called directly by m_reader, so no need to longjmp.

This method, headerAvailable, is called for each frame of a PNG, but
the setup code (e.g. setSize) only needs to be done once for the entire
image. Separate the pieces that only need to be done once from
headerAvailable. This makes it more clear that there is no need to
longjmp; use a boolean return instead. Now if setSize fails, no part
of m_reader will be accessed.

Move the extra size check into a new override of setSize, and move the
color space setup code to its own method.

Add a test image that reports a size that is too big for
ImageDecoder::setSize. Attempting to decode the size should fail.

BUG=702934
==========

to

==========
Refactor PNGImageDecoder's call to setSize

In the existing code, if setSize fails, PNGImageDecoder attempts to
dereference the png_structp owned by m_reader. But setSize calls
setFailed on failure, deleting m_reader in the process.

As it turns out, it is unnecessary to dereference the png_structp in
this case anyway. The intent is to call longjmp in order to stop
libpng's processing, but this block of code is only executed when this
method is called directly by m_reader, so no need to longjmp.

This method, headerAvailable, is called for each frame of a PNG, but
the setup code (e.g. setSize) only needs to be done once for the entire
image. Separate the pieces that only need to be done once from
headerAvailable. This makes it more clear that there is no need to
longjmp; use a boolean return instead. Now if setSize fails, no part
of m_reader will be accessed.

Move the extra size check into a new override of setSize, and move the
color space setup code to its own method.

Add a test image that reports a size that is too big for
ImageDecoder::setSize. Attempting to decode the size should fail.

BUG=702934

Review-Url: https://codereview.chromium.org/2766263002
Cr-Commit-Position: refs/heads/master@{#459074}
Committed:
https://chromium.googlesource.com/chromium/src/+/fab1444fe6528ac403acf2894549...
==========

[commit-bot: I haz the power, 2017-03-23 14:18:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/fab1444fe6528ac403acf2894549...