Ensure observers are notified when an image fails to decode.

Ensure observers are notified when an image fails to decode.

BUG=610990
TEST=http/tests/images/restyle-decode-error.html

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

[Nate Chapin, 2016-05-24 23:28:10]

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

[commit-bot: I haz the power, 2016-05-24 23:28:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009103002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009103002/1

[Nate Chapin, 2016-05-24 23:33:35]

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

[Nate Chapin, 2016-05-24 23:33:36]

eae, would you mind giving me a sanity check here?

This patch corrects a simple mistake: when we modify ImageResource::m_image (in
this case, by clearing it), we should notifyObservers().
https://crrev.com/61e34ff7dd4ac48b8c4275eb3f541ebfb8a50266 changed the image
decode error case from calling error(), which in turn calls notifyObservers(),
to clear(), which just updates ImageResource's state. This patch ensures that,
instead of early-returning in this case, the decode error case falls through and
calls notifyObservers().

This all makes sense to me in the abstract, but I have no clue why failing to
call notifyObservers() leads to a use-after-free in LayoutImage. Can you (or
someone else) shed light on that, and if not, how worried should I be that I'm
papering over a deeper bug?

[eae, 2016-05-24 23:43:35]

On 2016/05/24 23:33:36, Nate Chapin wrote:
> eae, would you mind giving me a sanity check here?
> 
> This patch corrects a simple mistake: when we modify ImageResource::m_image
(in
> this case, by clearing it), we should notifyObservers().
> https://crrev.com/61e34ff7dd4ac48b8c4275eb3f541ebfb8a50266 changed the image
> decode error case from calling error(), which in turn calls notifyObservers(),
> to clear(), which just updates ImageResource's state. This patch ensures that,
> instead of early-returning in this case, the decode error case falls through
and
> calls notifyObservers().
> 
> This all makes sense to me in the abstract, but I have no clue why failing to
> call notifyObservers() leads to a use-after-free in LayoutImage. Can you (or
> someone else) shed light on that, and if not, how worried should I be that I'm
> papering over a deeper bug?

I'm not overly familiar with the image resource code I'm afraid.
I'm guessing it has to do with the lifetime of the observer though. There is
likely a deeper underlying problem that is exposed by a failure to call notify.

[commit-bot: I haz the power, 2016-05-25 01:47:51]

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

[commit-bot: I haz the power, 2016-05-25 01:47:51]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 01:48:06]

Dry run: None

[eae, 2016-05-25 18:46:20]

LGTM

[Nate Chapin, 2016-05-25 21:58:02]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2016-05-25 21:59:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009103002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009103002/1

[Nate Chapin, 2016-05-25 22:00:24]

On 2016/05/24 23:43:35, eae wrote:
> On 2016/05/24 23:33:36, Nate Chapin wrote:
> > eae, would you mind giving me a sanity check here?
> > 
> > This patch corrects a simple mistake: when we modify ImageResource::m_image
> (in
> > this case, by clearing it), we should notifyObservers().
> > https://crrev.com/61e34ff7dd4ac48b8c4275eb3f541ebfb8a50266 changed the image
> > decode error case from calling error(), which in turn calls
notifyObservers(),
> > to clear(), which just updates ImageResource's state. This patch ensures
that,
> > instead of early-returning in this case, the decode error case falls through
> and
> > calls notifyObservers().
> > 
> > This all makes sense to me in the abstract, but I have no clue why failing
to
> > call notifyObservers() leads to a use-after-free in LayoutImage. Can you (or
> > someone else) shed light on that, and if not, how worried should I be that
I'm
> > papering over a deeper bug?
> 
> I'm not overly familiar with the image resource code I'm afraid.
> I'm guessing it has to do with the lifetime of the observer though. There is
> likely a deeper underlying problem that is exposed by a failure to call
notify.

I'm going to go ahead and get this landed, and fix any additional underlying
problems I find as a separate patch.

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

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-25 22:06:39]

Description was changed from

==========
Ensure observers are notified when an image fails to decode.

BUG=610990
TEST=http/tests/images/restyle-decode-error.html
==========

to

==========
Ensure observers are notified when an image fails to decode.

BUG=610990
TEST=http/tests/images/restyle-decode-error.html

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

[commit-bot: I haz the power, 2016-05-25 22:06:40]

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