ICO: Skip checking declared entry bounds when file is completelly received.

ICO: Skip checking declared entry bounds when file is completelly received.

It is relativelly common that the last entry offset + bytesize is two bytes over
file byte size. When calculating frame count, return entry count and allow
decoder to decode file.

BUG=653075
Committed: https://crrev.com/c3f6d43c1b2d245063da70a1d49247e8e2d5ee21
Cr-Commit-Position: refs/heads/master@{#423398}

[aleksandar.stojiljkovic, 2016-10-05 18:07:21]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ pkasting@chromium.org, scroggo@chromium.org

[scroggo_chromium, 2016-10-05 18:53:38]

lgtm

[Peter Kasting, 2016-10-05 19:03:27]

LGTM

https://codereview.chromium.org/2391073003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2391073003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:135:
// See crbug.com/653075.
Nit: I'd like this comment to say more about why we're doing this rather than
kicking to an external bug, so it's clear to a reader (it wasn't clear to me
without looking around more).  How about refactoring like this:

  // If the file is incomplete, return the length of the sequence of completely
  // received frames.  We don't do this when the file is fully received, since
  // some ICOs have entries whose claimed offset + size extends past the end of
  // the file, and we still want to display these if they don't trigger decoding
  // failures elsewhere.
  if (!isAllDataReceived()) {
    for (size_t i = 0; i < m_dirEntries.size(); ++i) {
      const IconDirectoryEntry& dirEntry = m_dirEntries[i];
      if ((dirEntry.m_imageOffset + dirEntry.m_byteSize) > m_data->size())
        return i;
    }
  }

  return m_dirEntries.size();

[aleksandar.stojiljkovic, 2016-10-05 19:17:07]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2016-10-05 19:17:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
scroggo@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2391073003/#ps20001
(title: "Comment and refactor - thanks pkasting@.")

[commit-bot: I haz the power, 2016-10-05 19:17:29]

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, 2016-10-05 19:51:47]

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

[commit-bot: I haz the power, 2016-10-05 19:51:48]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[aleksandar.stojiljkovic, 2016-10-05 20:47:41]

ico file that was used in test was valid (works fine in GIMP, Safari and
Firefox). Replacing with bad.ico from
src/third_party/WebKit/LayoutTests/fast/images/resources.
Related invalid.ico was added recently in crrev.com/2116803002. Anyway, test
passes with this new invalid.ico as expected.

[aleksandar.stojiljkovic, 2016-10-05 20:54:24]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ jochen@chromium.org, rockot@chromium.org

[aleksandar.stojiljkovic, 2016-10-05 20:54:25]

Patch Set 3 : ico used in test was valid, replacing. 
> On 2016/10/05 20:47:41, aleksandar.stojiljkovic wrote:
> ico file that was used in test was valid (works fine in GIMP, Safari and
> Firefox). Replacing with bad.ico from
> src/third_party/WebKit/LayoutTests/fast/images/resources.
> Related invalid.ico was added recently in crrev.com/2116803002. Anyway, test
> passes with this new invalid.ico as expected.

I dont know if there is a need for additional OWNER's review for file change in
content/test/data, but adding 2116803002's reviewers to verify if this
invalid.ico replacement is fine:
+jochen@, +rockot@
Thanks.

[Ken Rockot(use gerrit already), 2016-10-05 22:06:03]

Can you please explain the difference in invalid.ico? What is changing here?

[aleksandar.stojiljkovic, 2016-10-05 22:40:41]

On 2016/10/05 22:06:03, Ken Rockot wrote:
> Can you please explain the difference in invalid.ico? What is changing here?

Different file is used - I took really invalid file from Webkit (bad.ico) and
replaced content/test/data/invalid.ico.

The reason:
653075 is regression in decoding ICO files rendering some valid files as
invalid.
Current content/test/data/invalid.ico was one of those - while chrome code said
it was invalid, it wasn't (displayed properly elsewhere).
I found really invalid file in Webkit (bad.ico) and copied it over previous
content/test/data/invalid.ico.

[Ken Rockot(use gerrit already), 2016-10-05 22:42:09]

On 2016/10/05 at 22:40:41, aleksandar.stojiljkovic wrote:
> On 2016/10/05 22:06:03, Ken Rockot wrote:
> > Can you please explain the difference in invalid.ico? What is changing here?
> 
> Different file is used - I took really invalid file from Webkit (bad.ico) and
replaced content/test/data/invalid.ico.
> 
> The reason:
> 653075 is regression in decoding ICO files rendering some valid files as
invalid.
> Current content/test/data/invalid.ico was one of those - while chrome code
said it was invalid, it wasn't (displayed properly elsewhere).
> I found really invalid file in Webkit (bad.ico) and copied it over previous
content/test/data/invalid.ico.

Thanks for the explanation! LGTM

[aleksandar.stojiljkovic, 2016-10-05 22:43:28]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2016-10-05 22:43:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
scroggo@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2391073003/#ps40001
(title: "ico used in test was valid, replacing.")

[commit-bot: I haz the power, 2016-10-05 22:43:57]

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, 2016-10-06 02:21:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-06 02:23:28]

Description was changed from

==========
ICO: Skip checking declared entry bounds when file is completelly received.

It is relativelly common that the last entry offset + bytesize is two bytes over
file byte size. When calculating frame count, return entry count and allow
decoder to decode file.

BUG=653075
==========

to

==========
ICO: Skip checking declared entry bounds when file is completelly received.

It is relativelly common that the last entry offset + bytesize is two bytes over
file byte size. When calculating frame count, return entry count and allow
decoder to decode file.

BUG=653075

Committed: https://crrev.com/c3f6d43c1b2d245063da70a1d49247e8e2d5ee21
Cr-Commit-Position: refs/heads/master@{#423398}
==========

[commit-bot: I haz the power, 2016-10-06 02:23:29]

Patchset 3 (id:??) landed as
https://crrev.com/c3f6d43c1b2d245063da70a1d49247e8e2d5ee21
Cr-Commit-Position: refs/heads/master@{#423398}