gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

Previously, ToImageSkia, ToUIImage and ToNSImage would assume the
scoped_ptr they pass to AddRepresentation would remain valid after that
method returns. This *should* be the case, but it is poor form to assume
it. Now AddRepresentation returns a valid pointer and the callers return
that, rather than assuming the scoped_ptr stays alive.

BUG=590882
Committed: https://crrev.com/ce72b2661074ea7d583a276248d50e9763df5ede
Cr-Commit-Position: refs/heads/master@{#381607}

[Matt Giuca, 2016-03-04 06:19:42]

mgiuca@chromium.org changed reviewers:
+ rsesek@chromium.org

[Matt Giuca, 2016-03-04 06:19:42]

Hi Robert, what are your thoughts on this?

See my analysis in https://crbug.com/590882#c4.

[Robert Sesek, 2016-03-04 17:33:01]

Seems reasonable. LGTM.

[Matt Giuca, 2016-03-10 02:52:01]

Patchset #2 (id:20001) has been deleted

[Matt Giuca, 2016-03-15 02:21:03]

Description was changed from

==========
Fix use-after-free in gfx::Image.

ToImageSkia, ToUIImage and ToNSImage would insert an ImageRep into the
map, then return the pointer to the ImageRep. If the map already
contained a rep of that type, the new rep gets freed and the returned
pointer is dangling. Fixes so that if the new rep gets freed, it returns
a pointer to the existing rep in the map. Also adds a DCHECK for this
case, because it should not happen (but if it does, it is now safe).

It is possible that this can happen if (for example) ToImageSkia is
called from two threads at the same time (which should not happen, as
Image is not thread-safe).

BUG=590882
==========

to

==========
gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

Previously, ToImageSkia, ToUIImage and ToNSImage would assume the
scoped_ptr they pass to AddRepresentation would remain valid after that
method returns. This *should* be the case, but it is poor form to assume
it. Now AddRepresentation returns a valid pointer and the callers return
that, rather than assuming the scoped_ptr stays alive.

BUG=590882
==========

[Matt Giuca, 2016-03-15 02:21:15]

Description was changed from

==========
gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

Previously, ToImageSkia, ToUIImage and ToNSImage would assume the
scoped_ptr they pass to AddRepresentation would remain valid after that
method returns. This *should* be the case, but it is poor form to assume
it. Now AddRepresentation returns a valid pointer and the callers return
that, rather than assuming the scoped_ptr stays alive.

BUG=590882
==========

to

==========
gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

Previously, ToImageSkia, ToUIImage and ToNSImage would assume the
scoped_ptr they pass to AddRepresentation would remain valid after that
method returns. This *should* be the case, but it is poor form to assume
it. Now AddRepresentation returns a valid pointer and the callers return
that, rather than assuming the scoped_ptr stays alive.

BUG=590882
==========

[Matt Giuca, 2016-03-15 02:25:14]

Now that https://codereview.chromium.org/1773433002 has landed, I still think
this change is an improvement (but it's now just a refactor that should have no
behavioural difference). Updated the CL description. WDYT?

[Robert Sesek, 2016-03-16 20:15:45]

Yeah, I think this is still good. LGTM

[Matt Giuca, 2016-03-16 23:19:51]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2016-03-16 23:20:21]

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

[commit-bot: I haz the power, 2016-03-17 00:38:34]

Description was changed from

==========
gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

Previously, ToImageSkia, ToUIImage and ToNSImage would assume the
scoped_ptr they pass to AddRepresentation would remain valid after that
method returns. This *should* be the case, but it is poor form to assume
it. Now AddRepresentation returns a valid pointer and the callers return
that, rather than assuming the scoped_ptr stays alive.

BUG=590882
==========

to

==========
gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

Previously, ToImageSkia, ToUIImage and ToNSImage would assume the
scoped_ptr they pass to AddRepresentation would remain valid after that
method returns. This *should* be the case, but it is poor form to assume
it. Now AddRepresentation returns a valid pointer and the callers return
that, rather than assuming the scoped_ptr stays alive.

BUG=590882
==========

[commit-bot: I haz the power, 2016-03-17 00:38:35]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-03-17 00:40:50]

Description was changed from

==========
gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

Previously, ToImageSkia, ToUIImage and ToNSImage would assume the
scoped_ptr they pass to AddRepresentation would remain valid after that
method returns. This *should* be the case, but it is poor form to assume
it. Now AddRepresentation returns a valid pointer and the callers return
that, rather than assuming the scoped_ptr stays alive.

BUG=590882
==========

to

==========
gfx::Image: Refactor to avoid assuming scoped_ptr lifetime.

Previously, ToImageSkia, ToUIImage and ToNSImage would assume the
scoped_ptr they pass to AddRepresentation would remain valid after that
method returns. This *should* be the case, but it is poor form to assume
it. Now AddRepresentation returns a valid pointer and the callers return
that, rather than assuming the scoped_ptr stays alive.

BUG=590882

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

[commit-bot: I haz the power, 2016-03-17 00:40:51]

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