Cancel image loads if decoding failed (attempt #2)

Cancel image loads if decoding failed (attempt #2)

'Cancel' isn't really the right word for what this change does.
In the event of an image decoding failure, ImageResource will
synthesize a call to ResourceLoader::didFinishLoading, killing
the actual network request but reporting it as a successful
completion to the rest of blink. This matches our traditional
behavior of decoding errors looking like a successful
resource load (for the most part).

This requires some plumbing changes to image decoding, because
the decoder selection logic doesn't report why it wasn't able to
create an ImageDecoder. It might be because insufficient data has
been received to sniff the image type, or it may be that we
definitely don't have a valid image type. This change exposes enough
information to tell the difference.

BUG=471272
TBR=peter@chromium.org

Committed: https://crrev.com/d3d417564b290e70dc50bab63dc22a8889924be3
Cr-Commit-Position: refs/heads/master@{#407555}

[Nate Chapin, 2016-07-22 22:03:11]

Description was changed from

==========
SizeAvailability enum


Sniff image type in a separate step from ImageDecoder creation


Plumb image decoder state


Cancel image loads if decoding failed.

BUG=
==========

to

==========
Cancel image loads if decoding failed (attempt #2)

'Cancel' isn't really the right word for what this change does.
In the event of an image decoding failure, ImageResource will
synthesize a call to ResourceLoader::didFinishLoading, killing
the actual network request but reporting it as a successful
completion to the rest of blink. This matches our traditional
behavior of decoding errors looking like a successful
resource load (for the most part).

This requires some plumbing changes to image decoding, because
the decoder selection logic doesn't report why it wasn't able to
create an ImageDecoder. It might be because insufficient data has
been received to sniff the image type, or it may be that we
definitely don't have a valid image type. This change exposes enough
information to tell the difference.

BUG=471272
==========

[Nate Chapin, 2016-07-22 23:18:24]

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

[commit-bot: I haz the power, 2016-07-22 23:18:41]

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, 2016-07-22 23:30:14]

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

[commit-bot: I haz the power, 2016-07-22 23:30:14]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Nate Chapin, 2016-07-25 17:10:54]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-07-25 17:11:13]

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-07-25 17:11:14]

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

[commit-bot: I haz the power, 2016-07-25 17:11:15]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Nate Chapin, 2016-07-25 17:11:55]

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

[commit-bot: I haz the power, 2016-07-25 17:12:27]

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, 2016-07-25 17:22:04]

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

[commit-bot: I haz the power, 2016-07-25 17:22:05]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Nate Chapin, 2016-07-25 19:01:53]

japhet@chromium.org changed reviewers:
+ pdr@chromium.org

[Nate Chapin, 2016-07-25 19:01:54]

UAF fix! See the diffs between patchsets 1 and 2 for the fix.

[pdr., 2016-07-25 19:05:57]

On 2016/07/25 at 19:01:54, japhet wrote:
> UAF fix! See the diffs between patchsets 1 and 2 for the fix.

Is that change covered by a test?

If so, LGTM

[Nate Chapin, 2016-07-25 19:26:49]

On 2016/07/25 19:05:57, pdr. wrote:
> On 2016/07/25 at 19:01:54, japhet wrote:
> > UAF fix! See the diffs between patchsets 1 and 2 for the fix.
> 
> Is that change covered by a test?
> 
> If so, LGTM

Yeah, I added http/tests/images/restyle-decode-error.html a month or two to
catch exactly this regression, and it did its job, though I still don't know how
it got through the CQ :/

[Nate Chapin, 2016-07-25 19:32:01]

Description was changed from

==========
Cancel image loads if decoding failed (attempt #2)

'Cancel' isn't really the right word for what this change does.
In the event of an image decoding failure, ImageResource will
synthesize a call to ResourceLoader::didFinishLoading, killing
the actual network request but reporting it as a successful
completion to the rest of blink. This matches our traditional
behavior of decoding errors looking like a successful
resource load (for the most part).

This requires some plumbing changes to image decoding, because
the decoder selection logic doesn't report why it wasn't able to
create an ImageDecoder. It might be because insufficient data has
been received to sniff the image type, or it may be that we
definitely don't have a valid image type. This change exposes enough
information to tell the difference.

BUG=471272
==========

to

==========
Cancel image loads if decoding failed (attempt #2)

'Cancel' isn't really the right word for what this change does.
In the event of an image decoding failure, ImageResource will
synthesize a call to ResourceLoader::didFinishLoading, killing
the actual network request but reporting it as a successful
completion to the rest of blink. This matches our traditional
behavior of decoding errors looking like a successful
resource load (for the most part).

This requires some plumbing changes to image decoding, because
the decoder selection logic doesn't report why it wasn't able to
create an ImageDecoder. It might be because insufficient data has
been received to sniff the image type, or it may be that we
definitely don't have a valid image type. This change exposes enough
information to tell the difference.

BUG=471272
TBR=peter@chromium.org
==========

[Nate Chapin, 2016-07-25 19:32:13]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-07-25 19:32: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, 2016-07-25 20:05:08]

Description was changed from

==========
Cancel image loads if decoding failed (attempt #2)

'Cancel' isn't really the right word for what this change does.
In the event of an image decoding failure, ImageResource will
synthesize a call to ResourceLoader::didFinishLoading, killing
the actual network request but reporting it as a successful
completion to the rest of blink. This matches our traditional
behavior of decoding errors looking like a successful
resource load (for the most part).

This requires some plumbing changes to image decoding, because
the decoder selection logic doesn't report why it wasn't able to
create an ImageDecoder. It might be because insufficient data has
been received to sniff the image type, or it may be that we
definitely don't have a valid image type. This change exposes enough
information to tell the difference.

BUG=471272
TBR=peter@chromium.org
==========

to

==========
Cancel image loads if decoding failed (attempt #2)

'Cancel' isn't really the right word for what this change does.
In the event of an image decoding failure, ImageResource will
synthesize a call to ResourceLoader::didFinishLoading, killing
the actual network request but reporting it as a successful
completion to the rest of blink. This matches our traditional
behavior of decoding errors looking like a successful
resource load (for the most part).

This requires some plumbing changes to image decoding, because
the decoder selection logic doesn't report why it wasn't able to
create an ImageDecoder. It might be because insufficient data has
been received to sniff the image type, or it may be that we
definitely don't have a valid image type. This change exposes enough
information to tell the difference.

BUG=471272
TBR=peter@chromium.org
==========

[commit-bot: I haz the power, 2016-07-25 20:05:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-25 20:07:24]

Description was changed from

==========
Cancel image loads if decoding failed (attempt #2)

'Cancel' isn't really the right word for what this change does.
In the event of an image decoding failure, ImageResource will
synthesize a call to ResourceLoader::didFinishLoading, killing
the actual network request but reporting it as a successful
completion to the rest of blink. This matches our traditional
behavior of decoding errors looking like a successful
resource load (for the most part).

This requires some plumbing changes to image decoding, because
the decoder selection logic doesn't report why it wasn't able to
create an ImageDecoder. It might be because insufficient data has
been received to sniff the image type, or it may be that we
definitely don't have a valid image type. This change exposes enough
information to tell the difference.

BUG=471272
TBR=peter@chromium.org
==========

to

==========
Cancel image loads if decoding failed (attempt #2)

'Cancel' isn't really the right word for what this change does.
In the event of an image decoding failure, ImageResource will
synthesize a call to ResourceLoader::didFinishLoading, killing
the actual network request but reporting it as a successful
completion to the rest of blink. This matches our traditional
behavior of decoding errors looking like a successful
resource load (for the most part).

This requires some plumbing changes to image decoding, because
the decoder selection logic doesn't report why it wasn't able to
create an ImageDecoder. It might be because insufficient data has
been received to sniff the image type, or it may be that we
definitely don't have a valid image type. This change exposes enough
information to tell the difference.

BUG=471272
TBR=peter@chromium.org

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

[commit-bot: I haz the power, 2016-07-25 20:07:26]

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