Fixed thread-unsafe use of gfx::Image in app shortcut creation.

Fixed thread-unsafe use of gfx::Image in app shortcut creation.

Refactored web_app::ShortcutInfo so it is always passed via a scoped_ptr
(always having exactly one owner). This allows it to be safely passed
between threads without having a data race on the favicon image's
reference count.

Removed TSan suppression on ImageFamily.

BUG=364006
Committed: https://crrev.com/b21fe8ee736f3c2ee95b1cf181ef0a678bfcbf05
Cr-Commit-Position: refs/heads/master@{#324360}

[Matt Giuca, 2015-03-25 02:57:14]

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

[Matt Giuca, 2015-03-25 02:57:14]

benwells: WDYT? This is sort of a straw-man proposal (suggested on the
chromium-dev thread about RefCounted thread safety). I think it's quite a severe
constraint to impose on ourselves that we put every object that contains a
non-thread-safe RefCounted in a scoped_ptr for passing between threads. As you
can see here it resulted in globally updating the usage pattern for
ShortcutInfo.

The other proposal is to just make gfx::Image thread-safe:
https://codereview.chromium.org/240293006/
which I think is going to be much less onerous on all the clients.

[benwells, 2015-03-25 04:34:13]

On 2015/03/25 02:57:14, Matt Giuca wrote:
> benwells: WDYT? This is sort of a straw-man proposal (suggested on the
> chromium-dev thread about RefCounted thread safety). I think it's quite a
severe
> constraint to impose on ourselves that we put every object that contains a
> non-thread-safe RefCounted in a scoped_ptr for passing between threads. As you
> can see here it resulted in globally updating the usage pattern for
> ShortcutInfo.
> 
> The other proposal is to just make gfx::Image thread-safe:
> https://codereview.chromium.org/240293006/
> which I think is going to be much less onerous on all the clients.

I haven't looked closely, but I don't really like this at all. What I don't like
is that it is error prone - the user has to know exactly when to pass the
shortcut info. This is the kind of thing which should happen auto magically.

Have we analysed the perf hit of making image thread safe? Or alternatively
investigated if there are image alternatives that are thread safe that you could
use?

[Matt Giuca, 2015-03-25 05:41:30]

> I haven't looked closely, but I don't really like this at all.

OK, that's fine. As I said, this is kind of a straw man. I don't really like it
either (but perhaps not for the same reason).

> What I don't like
> is that it is error prone - the user has to know exactly when to pass the
> shortcut info. This is the kind of thing which should happen auto magically.

I should have made it so you don't need to care *when* to pass the ShortcutInfo.
Because I've disabled copying, it's now pretty hard to screw this up. The images
inside the ShortcutInfo should now always have a reference count of exactly 1,
and it should always be safe to pass it between threads. The big problem is how
limiting this is; you can no longer pass a ShortcutInfo without having your own
copy of it be destroyed (as is the case in
CreateApplicationShortcutView::Accept).

> Have we analysed the perf hit of making image thread safe? Or alternatively
> investigated if there are image alternatives that are thread safe that you
could
> use?

No, I am not really clear on how to go about doing a proper perf analysis. But
that is the approach I am arguing for on the mailing list.

[benwells, 2015-03-25 06:03:33]

On 2015/03/25 05:41:30, Matt Giuca wrote:
> > I haven't looked closely, but I don't really like this at all.
> 
> OK, that's fine. As I said, this is kind of a straw man. I don't really like
it
> either (but perhaps not for the same reason).
> 
> > What I don't like
> > is that it is error prone - the user has to know exactly when to pass the
> > shortcut info. This is the kind of thing which should happen auto magically.
> 
> I should have made it so you don't need to care *when* to pass the
ShortcutInfo.
> Because I've disabled copying, it's now pretty hard to screw this up. The
images
> inside the ShortcutInfo should now always have a reference count of exactly 1,
> and it should always be safe to pass it between threads. The big problem is
how
> limiting this is; you can no longer pass a ShortcutInfo without having your
own
> copy of it be destroyed (as is the case in
> CreateApplicationShortcutView::Accept).

Right ok. Like I said I haven't looked closely. The thing about error prone was
e.g. one of the web_app:: methods taking a raw pointer and one taking a scoped
pointer, but that's not error prone, it should be pretty obvious what is needed
based on lifetime requirements.

Maybe it just feels unwieldy, but then again maybe it is a necessary evil.

> 
> > Have we analysed the perf hit of making image thread safe? Or alternatively
> > investigated if there are image alternatives that are thread safe that you
> could
> > use?
> 
> No, I am not really clear on how to go about doing a proper perf analysis. But
> that is the approach I am arguing for on the mailing list.

From the thread it sounds like gfx::Image has bigger issues with thread safety
than just the reference counting. Maybe the whole notion of using this class
across threads is flawed. Can you make a deep copy and send that over instead
(waves hands about), or something?

[Matt Giuca, 2015-03-25 06:18:21]

> From the thread it sounds like gfx::Image has bigger issues with thread safety
> than just the reference counting. Maybe the whole notion of using this class
> across threads is flawed. Can you make a deep copy and send that over instead
> (waves hands about), or something?

Thread-safe deep copies are not currently possible. Firstly because ImageStorage
is non-copyable, and secondly because ImageSkia's copy semantics is also
ref-counted. Also, if we have to make a deep copy of an image (to get around
thread safety issues), I think something is seriously wrong.

[benwells, 2015-03-25 07:01:36]

On 2015/03/25 06:18:21, Matt Giuca wrote:
> > From the thread it sounds like gfx::Image has bigger issues with thread
safety
> > than just the reference counting. Maybe the whole notion of using this class
> > across threads is flawed. Can you make a deep copy and send that over
instead
> > (waves hands about), or something?
> 
> Thread-safe deep copies are not currently possible. Firstly because
ImageStorage
> is non-copyable, and secondly because ImageSkia's copy semantics is also
> ref-counted. Also, if we have to make a deep copy of an image (to get around
> thread safety issues), I think something is seriously wrong.

I just looked up my vague memory of coming across this. On this cl:
https://chromiumcodereview.appspot.com/11198025, at sky's suggestion I deep
copied the ImageSkia and passed that around.

[Matt Giuca, 2015-03-25 07:08:56]

Ah I see. Didn't notice ImageSkia's DeepCopy() before. Well, we could add a
DeepCopy() method to ImageFamily and Image. Would still need the scoped_ptr
trick to get the new image over to the other thread without refcount
shenanigans, but it wouldn't require global use of scoped_ptr. Shall I have a go
at that then?

(Still find the idea of deep-copying an image unpalatable compared to fixing
thread safety, but at least it means only localized performance issues.)

[benwells, 2015-03-25 07:53:57]

On 2015/03/25 07:08:56, Matt Giuca wrote:
> Ah I see. Didn't notice ImageSkia's DeepCopy() before. Well, we could add a
> DeepCopy() method to ImageFamily and Image. Would still need the scoped_ptr
> trick to get the new image over to the other thread without refcount
> shenanigans, but it wouldn't require global use of scoped_ptr. Shall I have a
go
> at that then?
> 
> (Still find the idea of deep-copying an image unpalatable compared to fixing
> thread safety, but at least it means only localized performance issues.)

Deep copying to do image manipulation on another thread for performance reasons
does seem weird.

In the case I linked to, it was fine. It is a once off, user initiated action,
so it doesn't need to be fast - we just don't want it on the UI thread.

I don't think all cases are like that (although perhaps yours is) so we probably
need a better way to do things.

[Matt Giuca, 2015-03-25 22:36:55]

> Deep copying to do image manipulation on another thread for performance
reasons
> does seem weird.
> 
> In the case I linked to, it was fine. It is a once off, user initiated action,
> so it doesn't need to be fast - we just don't want it on the UI thread.
> 
> I don't think all cases are like that (although perhaps yours is) so we
probably
> need a better way to do things.

I think this code will mostly run in response to user action, but it probably
also runs when all the apps sync down. Still, that's not a huge overhead (since
it's one small icon copy in memory for each download of multiple kilobytes from
the web server), so I think doing a copy here is "OK" from a performance
standpoint. Just seems messy.

[Matt Giuca, 2015-03-26 00:44:07]

Wow. Just had a go at making gfx::Image deep-copyable. It's far too complex. I
got as far as having an Image backed by an ImageSkia deep-copyable, but it seems
that all the representations are ref-counted and you would need to add deep-copy
semantics to everything there.

I think we should just go ahead with the current approach. Please do a full
review.

[benwells, 2015-03-26 05:28:51]

https://codereview.chromium.org/1038573002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/1038573002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/create_application_shortcut_view.cc:402:
shortcut_info_.reset(new web_app::ShortcutInfo);
Why do this? Put another way, why is there a constraint to have shortcut_info_
not null?

[Matt Giuca, 2015-03-27 08:14:47]

Hmm, just realised I forgot to update the Windows and Mac versions of web_app_*.
So I will have to do that next week.

https://codereview.chromium.org/1038573002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/1038573002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/create_application_shortcut_view.cc:402:
shortcut_info_.reset(new web_app::ShortcutInfo);
Partly laziness :)

It would mean having to introduce null checks throughout this whole file. Seems
OK.

Done.

[benwells, 2015-03-30 01:00:43]

On 2015/03/27 08:14:47, Matt Giuca wrote:
> Hmm, just realised I forgot to update the Windows and Mac versions of
web_app_*.
> So I will have to do that next week.
> 
>
https://codereview.chromium.org/1038573002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/create_application_shortcut_view.cc (right):
> 
>
https://codereview.chromium.org/1038573002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/create_application_shortcut_view.cc:402:
> shortcut_info_.reset(new web_app::ShortcutInfo);
> Partly laziness :)
> 
> It would mean having to introduce null checks throughout this whole file.
Seems
> OK.
> 
> Done.

lgtm.

[Matt Giuca, 2015-03-31 00:16:31]

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

[Matt Giuca, 2015-03-31 00:16:32]

Ich. This got a LOT worse after I did the Windows and Mac integration
(especially Mac). +jackhou for Mac stuff.

As discussed, I think it is (regardless of thread safety) a good idea to make
ShortcutInfo non-copyable and update everything to pass it with either a
scoped_ptr or by reference. So I've gone ahead with that, updating throughout
the Mac code.

If we didn't want to commit to such a huge refactor, we could go back to having
ShortcutInfo be copyable. We would have to make sure that it NEVER gets copied
on the UI thread (before being passed to the FILE thread), and then it's safe to
copy once on the FILE thread. Then I could revert much of the changes,
especially on Mac. But I think that strategy is more risky since it involves
carefully avoiding copying a copyable object in certain situations. Better to
make it non-copyable.

PTAL.

[jackhou1, 2015-03-31 02:39:03]

mac lgtm

[benwells, 2015-04-01 02:01:33]

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.h (right):

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:53: // outlive the
WebAppShortcutCreator.
This is unfortunate. Do you need much info from the shortcut? Can you just copy
out what you need?

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:124: const ShortcutInfo& info_;
Maybe it would be better to use a raw pointer here. That is more typically
associated with 'owned by something else'.

[Matt Giuca, 2015-04-01 03:31:47]

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.h (right):

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:53: // outlive the
WebAppShortcutCreator.
Well, you do need |favicon|, which is the reason it is non-copyable in the first
place. Of course, favicon *is* copyable (with internal ref counting), so you
could copy all of the data fields including favicon into the
WebAppShortcutCreator. But then if you wanted to go down that route, you would
just make ShortcutInfo copyable again.

As I said in my previous comment, that *is* a reasonable thing to do, but it
makes it trickier to get threading right, since you might accidentally copy a
ShortcutInfo across a thread.

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:124: const ShortcutInfo& info_;
OK, I'll do that if you agree that we should still not take a copy of the data
in the above discussion.

[Matt Giuca, 2015-04-01 03:54:26]

Good now? (Assuming the mac still works.)

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.h (right):

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:53: // outlive the
WebAppShortcutCreator.
As discussed, not doing this.

https://codereview.chromium.org/1038573002/diff/360001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.h:124: const ShortcutInfo& info_;
On 2015/04/01 03:31:47, Matt Giuca wrote:
> OK, I'll do that if you agree that we should still not take a copy of the data
> in the above discussion.

Done.

[Matt Giuca, 2015-04-01 03:55:33]

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

[Matt Giuca, 2015-04-01 03:55:35]

msw: For chrome/browser/ui/views/create_application_shortcut_view owners.
Thanks!

[benwells, 2015-04-01 04:49:10]

lgtm

https://codereview.chromium.org/1038573002/diff/400001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/1038573002/diff/400001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.mm:638:
file_handlers_info_(file_handlers_info) {
Nit: wanna add DCHECK(shortcut_info) in here?

[Matt Giuca, 2015-04-01 06:28:38]

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

[Matt Giuca, 2015-04-01 06:28:39]

The patchset sent to the CQ was uploaded after l-g-t-m from jackhou@chromium.org
Link to the patchset: https://codereview.chromium.org/1038573002/#ps400001
(title: "Attempt to fix Mac compile.")

[commit-bot: I haz the power, 2015-04-01 06:29:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1038573002/400001

[commit-bot: I haz the power, 2015-04-01 06:38:51]

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

[commit-bot: I haz the power, 2015-04-01 06:38:53]

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

[Matt Giuca, 2015-04-01 07:04:25]

https://codereview.chromium.org/1038573002/diff/400001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/1038573002/diff/400001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_mac.mm:638:
file_handlers_info_(file_handlers_info) {
On 2015/04/01 04:49:10, benwells wrote:
> Nit: wanna add DCHECK(shortcut_info) in here?

Done.

[Matt Giuca, 2015-04-01 07:19:33]

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

[Matt Giuca, 2015-04-01 07:19:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
jackhou@chromium.org, benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/1038573002/#ps440001
(title: "Fix ChromeOS compile.")

[commit-bot: I haz the power, 2015-04-01 07:19:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1038573002/440001

[commit-bot: I haz the power, 2015-04-01 07:29:48]

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

[commit-bot: I haz the power, 2015-04-01 07:29:49]

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

[msw, 2015-04-01 16:23:00]

lgtm with nits and qs.

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/ui/view...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/ui/view...
chrome/browser/ui/views/create_application_shortcut_view.cc:376: // Can happen
if the shortcut data is not yet loaded.
nit: "The shortcut info may be null if"...

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/ui/view...
chrome/browser/ui/views/create_application_shortcut_view.cc:504: // Can happen
if the dialog has already been accepted.
ditto nit: "The shortcut info may be null if"...

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
chrome/browser/web_applications/web_app.cc:112: image_skia.MakeThreadSafe();
q: Is this still necessary now?

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:61: // despite ImageFamily having a
non-thread-safe reference count.
Hmm, tough spot, but I suppose a thread-safe gfx::Image[Family] isn't a simple
matter, can you link to the chromium-dev thread you mention?

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_win.cc:679: shortcut_info->title, NULL,
NULL);
nit: nullptr

[Matt Giuca, 2015-04-01 22:52:17]

I'm on vacation and can't make code changes now. I'll do the other things you
suggested next week. (I think it's probably best to wait until branch point
before landing this CL anyway.)

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
chrome/browser/web_applications/web_app.cc:112: image_skia.MakeThreadSafe();
Yes. This CL fixes thread safety issues regarding the refcount on gfx::Image. It
is still necessary for the ImageSkia to be thread-safe.

(In fact, it is probably not necessary, since the ImageSkia will not be accessed
on two threads at once, but the DCHECK doesn't care; it just uses
CalledOnValidThread which fails if an object is ever accessed from two threads,
even if properly synchronized.)

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:61: // despite ImageFamily having a
non-thread-safe reference count.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8LqVoXQ_2bo/cW0tH...

Yeah, believe me. I've made about 3 or 4 alternative CLs to fix this issue in
various ways. This was the least awful.

Making gfx::Image thread-safe I did here:
https://codereview.chromium.org/240293006/
and discussed that on the above thread. It wasn't well received. Basically
people didn't like the idea of unnecessarily making something thread-safe
universally when it was possible to just ensure it is only accessed on a single
thread.

I think having non-copyable classes is generally considered better practice
anyhow.

[msw, 2015-04-01 23:35:02]

On 2015/04/01 22:52:17, Matt Giuca (OOO until April 9) wrote:
> I'm on vacation and can't make code changes now. I'll do the other things you
> suggested next week. (I think it's probably best to wait until branch point
> before landing this CL anyway.)
> 
>
https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
> File chrome/browser/web_applications/web_app.cc (right):
> 
>
https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
> chrome/browser/web_applications/web_app.cc:112: image_skia.MakeThreadSafe();
> Yes. This CL fixes thread safety issues regarding the refcount on gfx::Image.
It
> is still necessary for the ImageSkia to be thread-safe.
> 
> (In fact, it is probably not necessary, since the ImageSkia will not be
accessed
> on two threads at once, but the DCHECK doesn't care; it just uses
> CalledOnValidThread which fails if an object is ever accessed from two
threads,
> even if properly synchronized.)
> 
>
https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
> File chrome/browser/web_applications/web_app.h (right):
> 
>
https://codereview.chromium.org/1038573002/diff/440001/chrome/browser/web_app...
> chrome/browser/web_applications/web_app.h:61: // despite ImageFamily having a
> non-thread-safe reference count.
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/8LqVoXQ_2bo/cW0tH...
> 
> Yeah, believe me. I've made about 3 or 4 alternative CLs to fix this issue in
> various ways. This was the least awful.
> 
> Making gfx::Image thread-safe I did here:
> https://codereview.chromium.org/240293006/
> and discussed that on the above thread. It wasn't well received. Basically
> people didn't like the idea of unnecessarily making something thread-safe
> universally when it was possible to just ensure it is only accessed on a
single
> thread.
> 
> I think having non-copyable classes is generally considered better practice
> anyhow.

That thread convinced me that gfx::Image, etc. shouldn't be copyable.
This patch makes sense, and still lgtm (with nits you can ignore if needed).

[Matt Giuca, 2015-04-02 00:00:58]

Thanks. I'll just wait a week anyhow to avoid congestion with branch point.

[Matt Giuca, 2015-04-08 23:32:17]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-04-08 23:32:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
jackhou@chromium.org, benwells@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1038573002/#ps460001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-04-08 23:34:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1038573002/460001

[commit-bot: I haz the power, 2015-04-09 04:00:54]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2015-04-09 04:02:30]

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