gfx::Image is now thread-safe.

gfx::Image is now thread-safe.

Its internal storage now uses RefCountedThreadSafe, and a mutex to guard
against access to mutable fields.

Also took this opportunity to fix up const correctness of some of the
internal code, and add documentation to the header file.

BUG=468010, 364006

[Matt Giuca, 2014-04-17 06:27:07]

Hi Robert,

This is an early WIP patch (it hasn't been tested beyond manually verifying that
the browser still works). But before putting work into testing (tsan,
performance, etc), I want to get a rough idea of whether this is a good idea.

Please see the bug for details. We need to pass an Image from the UI thread to
the FILE thread, and ask for its PNG representation on the FILE thread. We also
add and release references on both sides, so it needs to be thread safe.

This is kind of a shotgun approach (making all Images thread safe everywhere);
we could be more careful about our usage of Image but it was quite complicated
to do so and I thought it might be more useful to fix the problem at its source.
WDYT?

[Robert Sesek, 2014-04-17 16:56:58]

I think this change is doing two things at once, and I think only one may be
necessary:

1. Changing the ImageStorage from RefCounted to RefCountedThreadSafe supports
the cheap-copy semantics of gfx::Image. This makes it possible safely copy
gfx::Image objects between threads, since the refcount will be atomically
mutated. It adds a finer distinction between getting a |gfx::Image| and a |const
gfx::Image&|, but it's one that makes sense.

2. Changing ImageStorage to be have synchronized access changes the thread
safety guarantees of the class. I'm not sure this is necessary, and I'm hesitant
to add it. While bouncing gfx::Images across threads for loading and/or
manipulation can be made better by fixing (1), I don't think supporting
concurrent access falls under that same umbrella.

For either/both of these, I'm also concerned about the performance implications.
gfx::Image is used heavily on the UI thread from the ResourceBundle, and adding
unnecessary synchronization for accessing it seems unfortunate.

Is it really necessary to be able to add or modify representations in the same
gfx::Image from different threads at the same time (2)? Or, is it sufficient to
make sure that it can be safely copied to another thread, and then back to the
original, with serialized data member access (1)?

https://codereview.chromium.org/240293006/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

https://codereview.chromium.org/240293006/diff/1/ui/gfx/image/image.h#newcode52
ui/gfx/image/image.h:52: // An image, backed by zero or more equivalent
representations. When a specific
The comment for this class is at the top of the file.

[Matt Giuca, 2015-04-09 00:25:14]

Finally closing this issue.

Instead of making gfx::Image thread-safe, I am fixing the thread-unsafe uses of
it in app shortcut creation code (https://codereview.chromium.org/1038573002).
See the discussion at http://crbug.com/468010.