Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on another thread, the copy operation
of gfx::Image causes a data race on the ref count.
E.g. web_apps::internals::UpdatePlatformShortcuts() on Windows calls
gfx::ImageFamily::CreateExact() via IconUtil::CreateIconFileFromImageFamily()
on FILE thread.

BUG=688072, 468010
Review-Url: https://codereview.chromium.org/2707993005
Cr-Commit-Position: refs/heads/master@{#452740}
Committed: https://chromium.googlesource.com/chromium/src/+/06203bafdd105bf358fd8f216fd77ae0053f4ec7

[tzik, 2017-02-22 08:49:15]

Description was changed from

==========
.
==========

to

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact
==========

[tzik, 2017-02-22 09:07:21]

Description was changed from

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact
==========

to

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on a blocking pool thread, the
copy operation of gfx::Image causes a data race on the ref count.
==========

[tzik, 2017-02-22 09:07:58]

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

[tzik, 2017-02-22 09:08:04]

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

[commit-bot: I haz the power, 2017-02-22 09:08:44]

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

[tzik, 2017-02-22 09:08:54]

Description was changed from

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on a blocking pool thread, the
copy operation of gfx::Image causes a data race on the ref count.
==========

to

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on a blocking pool thread, the
copy operation of gfx::Image causes a data race on the ref count.

BUG=688072, 468010
==========

[commit-bot: I haz the power, 2017-02-22 09:18:00]

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

[commit-bot: I haz the power, 2017-02-22 09:18:01]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tzik, 2017-02-22 09:24:23]

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

[commit-bot: I haz the power, 2017-02-22 09:25:26]

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

[tzik, 2017-02-22 09:38:22]

PTAL

[tzik, 2017-02-22 09:58:21]

Description was changed from

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on a blocking pool thread, the
copy operation of gfx::Image causes a data race on the ref count.

BUG=688072, 468010
==========

to

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on another thread, the copy operation
of gfx::Image causes a data race on the ref count.
E.g. web_apps::internals::UpdatePlatformShortcuts() on Windows calls
gfx::ImageFamily::CreateExact() via IconUtil::CreateIconFileFromImageFamily()
on FILE thread.

BUG=688072, 468010
==========

[commit-bot: I haz the power, 2017-02-22 10:44:43]

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

[commit-bot: I haz the power, 2017-02-22 10:44:44]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-02-22 18:44:46]

https://codereview.chromium.org/2707993005/diff/20001/ui/gfx/image/image_fami...
File ui/gfx/image/image_family.cc (right):

https://codereview.chromium.org/2707993005/diff/20001/ui/gfx/image/image_fami...
ui/gfx/image/image_family.cc:121: std::unique_ptr<gfx::ImageSkia>
image_skia(image->CopyImageSkia());
I'd leave a comment here as to why this is done.

[tzik, 2017-02-22 22:01:16]

https://codereview.chromium.org/2707993005/diff/20001/ui/gfx/image/image_fami...
File ui/gfx/image/image_family.cc (right):

https://codereview.chromium.org/2707993005/diff/20001/ui/gfx/image/image_fami...
ui/gfx/image/image_family.cc:121: std::unique_ptr<gfx::ImageSkia>
image_skia(image->CopyImageSkia());
On 2017/02/22 18:44:46, Robert Sesek wrote:
> I'd leave a comment here as to why this is done.

Done.

[Matt Giuca, 2017-02-22 22:30:26]

I had a few concerns to think about:
1. The fact that this assumes the Image is an ImageSkia (if it isn't, it still
works, but it converts to an ImageSkia).
2. The fact that GetBest and friends still return a pointer to an image rather
than making a copy makes this inconsistent.

I think these are both mitigated by the fact that CreateExact *can* create a new
SkBitmap, so this change is just making it behave consistently regardless of
whether there is a matching size image.

So, lgtm.

[Robert Sesek, 2017-02-23 20:37:17]

LGTM

[tzik, 2017-02-23 23:50:02]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-02-23 23:50:48]

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-02-24 03:04:30]

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

[commit-bot: I haz the power, 2017-02-24 03:04:31]

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

[tzik, 2017-02-24 03:08:01]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-02-24 03:08:42]

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-02-24 03:35:29]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487905681801420,
"parent_rev": "474e1f148ab83dde92b93a93e53cf83d77b9be13", "commit_rev":
"06203bafdd105bf358fd8f216fd77ae0053f4ec7"}

[commit-bot: I haz the power, 2017-02-24 03:36:09]

Description was changed from

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on another thread, the copy operation
of gfx::Image causes a data race on the ref count.
E.g. web_apps::internals::UpdatePlatformShortcuts() on Windows calls
gfx::ImageFamily::CreateExact() via IconUtil::CreateIconFileFromImageFamily()
on FILE thread.

BUG=688072, 468010
==========

to

==========
Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on another thread, the copy operation
of gfx::Image causes a data race on the ref count.
E.g. web_apps::internals::UpdatePlatformShortcuts() on Windows calls
gfx::ImageFamily::CreateExact() via IconUtil::CreateIconFileFromImageFamily()
on FILE thread.

BUG=688072, 468010

Review-Url: https://codereview.chromium.org/2707993005
Cr-Commit-Position: refs/heads/master@{#452740}
Committed:
https://chromium.googlesource.com/chromium/src/+/06203bafdd105bf358fd8f216fd7...
==========

[commit-bot: I haz the power, 2017-02-24 03:36:10]

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