Make web_app::ShortcutInfo RefCountedThreadSafe (1)

Make web_app::ShortcutInfo RefCountedThreadSafe

This is another approach of http://crrev.com/2703283005/. Either
this or another is enough to fix the issue.

web_app::ShortcutInfo used in cr/b/web_applications needs to be
destroyed on UI thread, because it has a reference to a ImageStorage
via gfx::ImageFamily. Since gfx::internal::ImageStorage has a
non-thread-safe ref count, created on UI thread, and shared by
others on UI thread, it needs on UI thread too on destruction.

BUG=688072

[tzik, 2017-02-27 07:26:08]

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

[commit-bot: I haz the power, 2017-02-27 07:26:27]

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-27 07:28:28]

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

[commit-bot: I haz the power, 2017-02-27 07:28:35]

Dry run: 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-27 07:40:33]

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

[commit-bot: I haz the power, 2017-02-27 07:40:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tzik, 2017-02-27 07:44:30]

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

[commit-bot: I haz the power, 2017-02-27 07:44:41]

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-27 07:57:14]

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

[commit-bot: I haz the power, 2017-02-27 07:57:31]

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-27 08:06:35]

Description was changed from

==========
Make web_app::ShortcutInfo :RefCountedThreadSafe

BUG=
==========

to

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

BUG=
==========

[commit-bot: I haz the power, 2017-02-27 09:00:39]

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

[commit-bot: I haz the power, 2017-02-27 09:00:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tzik, 2017-02-28 05:31:13]

Description was changed from

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

BUG=
==========

to

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

12345678901234567890123456789012345678901234567890123456789012

This is another approach to http://crrev.com/2703283005/. Either
this or another is enough to fix the problem.

web_app::ShortcutInfo used in cr/b/web_applications needs to be
destroyed on UI thread, because it has a reference to a ImageStorage
via gfx::ImageFamily. Since gfx::internal::ImageStorage has a
non-thread-safe ref count, created on UI thread, and shared by
others on UI thread, it needs on UI thread too on destruction.

BUG=
==========

[tzik, 2017-02-28 06:01:43]

Description was changed from

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

12345678901234567890123456789012345678901234567890123456789012

This is another approach to http://crrev.com/2703283005/. Either
this or another is enough to fix the problem.

web_app::ShortcutInfo used in cr/b/web_applications needs to be
destroyed on UI thread, because it has a reference to a ImageStorage
via gfx::ImageFamily. Since gfx::internal::ImageStorage has a
non-thread-safe ref count, created on UI thread, and shared by
others on UI thread, it needs on UI thread too on destruction.

BUG=
==========

to

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

This is another approach of http://crrev.com/2703283005/. Either
this or another is enough to fix the problem.

web_app::ShortcutInfo used in cr/b/web_applications needs to be
destroyed on UI thread, because it has a reference to a ImageStorage
via gfx::ImageFamily. Since gfx::internal::ImageStorage has a
non-thread-safe ref count, created on UI thread, and shared by
others on UI thread, it needs on UI thread too on destruction.

BUG=
==========

[tzik, 2017-02-28 06:02:45]

Description was changed from

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

This is another approach of http://crrev.com/2703283005/. Either
this or another is enough to fix the problem.

web_app::ShortcutInfo used in cr/b/web_applications needs to be
destroyed on UI thread, because it has a reference to a ImageStorage
via gfx::ImageFamily. Since gfx::internal::ImageStorage has a
non-thread-safe ref count, created on UI thread, and shared by
others on UI thread, it needs on UI thread too on destruction.

BUG=
==========

to

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

This is another approach of http://crrev.com/2703283005/. Either
this or another is enough to fix the problem.

web_app::ShortcutInfo used in cr/b/web_applications needs to be
destroyed on UI thread, because it has a reference to a ImageStorage
via gfx::ImageFamily. Since gfx::internal::ImageStorage has a
non-thread-safe ref count, created on UI thread, and shared by
others on UI thread, it needs on UI thread too on destruction.

BUG=688072
==========

[tzik, 2017-02-28 06:02:45]

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

[tzik, 2017-02-28 06:16:13]

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

[commit-bot: I haz the power, 2017-02-28 06:16:32]

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-28 07:01:05]

Description was changed from

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

This is another approach of http://crrev.com/2703283005/. Either
this or another is enough to fix the problem.

web_app::ShortcutInfo used in cr/b/web_applications needs to be
destroyed on UI thread, because it has a reference to a ImageStorage
via gfx::ImageFamily. Since gfx::internal::ImageStorage has a
non-thread-safe ref count, created on UI thread, and shared by
others on UI thread, it needs on UI thread too on destruction.

BUG=688072
==========

to

==========
Make web_app::ShortcutInfo RefCountedThreadSafe

This is another approach of http://crrev.com/2703283005/. Either
this or another is enough to fix the issue.

web_app::ShortcutInfo used in cr/b/web_applications needs to be
destroyed on UI thread, because it has a reference to a ImageStorage
via gfx::ImageFamily. Since gfx::internal::ImageStorage has a
non-thread-safe ref count, created on UI thread, and shared by
others on UI thread, it needs on UI thread too on destruction.

BUG=688072
==========

[commit-bot: I haz the power, 2017-02-28 07:15:01]

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

[commit-bot: I haz the power, 2017-02-28 07:15:02]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-02-28 23:30:06]

I like this one :). So lgtm. I see this approach as giving a better guarantee
that the same mistake isn't simply made again in future, since the interaction
is so subtle.

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

https://codereview.chromium.org/2721553002/diff/100001/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:71: // despite ImageFamily having a
non-thread-safe reference count.
I suggest moving this up to a class comment, with a mention of how ImageFamily
may contain a gfx::internal::ImageStorage shared with other code on the UI
thread, so must always be destroyed there.

[tzik, 2017-03-01 05:24:19]

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

[commit-bot: I haz the power, 2017-03-01 05:24:37]

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

[tzik, 2017-03-01 05:31:34]

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

https://codereview.chromium.org/2721553002/diff/100001/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:71: // despite ImageFamily having a
non-thread-safe reference count.
On 2017/02/28 23:30:06, tapted wrote:
> I suggest moving this up to a class comment, with a mention of how ImageFamily
> may contain a gfx::internal::ImageStorage shared with other code on the UI
> thread, so must always be destroyed there.

Acknowledged.

[tzik, 2017-03-01 05:32:30]

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

https://codereview.chromium.org/2721553002/diff/100001/chrome/browser/web_app...
chrome/browser/web_applications/web_app.h:71: // despite ImageFamily having a
non-thread-safe reference count.
On 2017/02/28 23:30:06, tapted wrote:
> I suggest moving this up to a class comment, with a mention of how ImageFamily
> may contain a gfx::internal::ImageStorage shared with other code on the UI
> thread, so must always be destroyed there.

Done.

[commit-bot: I haz the power, 2017-03-01 06:13:17]

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

[commit-bot: I haz the power, 2017-03-01 06:13:18]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2017-03-01 08:52:49]

Hi, sorry I didn't get back on this earlier in the day.

As I wrote on https://codereview.chromium.org/2703283005/, I think the
single-ownership model is much neater. Not sure what Trent means about "giving a
better guarantee
that the same mistake isn't simply made again in future" -- that CL sets up an
architecture where it's always deleted on the UI thread. If you wanted to, you
could also add a thread checker to the ShortcutInfo destructor.

Whereas this one just papers over the ownership by having it managed at runtime
-- very hard to reason about.

[tapted, 2017-03-01 10:18:07]

On 2017/03/01 08:52:49, Matt Giuca wrote:
> Hi, sorry I didn't get back on this earlier in the day.
> 
> As I wrote on https://codereview.chromium.org/2703283005/, I think the
> single-ownership model is much neater. Not sure what Trent means about "giving
a
> better guarantee
> that the same mistake isn't simply made again in future"
>  -- that CL sets up an
> architecture where it's always deleted on the UI thread.

Architecting a set of functions that happen to do a thing isn't the same as
having
a type that enforces that that thing must be done when using that type.

It's unfortunate that we don't have a non-refcounted wrapper that enforces the
same thing as content::BrowserThread::DeleteOnUIThread. refcounts are a
tradeoff, but
not a significant one IMO this isn't high-performance code.
Also base::BindStateBase is refcounted. Every call to
base::Bind is effectively adding a refcount around a ShortcutInfo anyway.

Without DeleteOnUIThread it's still easy to accidentally pass a ShortcutInfo to
another thread and have it deleted there. Remembering that you need to architect
your functions in a particular way is an unnecessary burden.

> If you wanted to, you
> could also add a thread checker to the ShortcutInfo destructor.

That won't help. The risk is that it *might* get deleted on the wrong thread
subject to some unpredictable data races that may or may not happen.

> 
> Whereas this one just papers over the ownership by having it managed at
runtime
> -- very hard to reason about.

I am an ObjC developer so maybe I have been desensitised to refcounting :p

[tapted, 2017-03-01 10:35:35]

On 2017/03/01 10:18:07, tapted wrote:
> On 2017/03/01 08:52:49, Matt Giuca wrote:
> > If you wanted to, you
> > could also add a thread checker to the ShortcutInfo destructor.
> 
> That won't help. The risk is that it *might* get deleted on the wrong thread
> subject to some unpredictable data races that may or may not happen.

ooh - actually I am mistaken, on ShortcutInfo, this could be useful :)

[Matt Giuca, 2017-03-01 23:40:54]

On 2017/03/01 10:18:07, tapted wrote:
> On 2017/03/01 08:52:49, Matt Giuca wrote:
> > Hi, sorry I didn't get back on this earlier in the day.
> > 
> > As I wrote on https://codereview.chromium.org/2703283005/, I think the
> > single-ownership model is much neater. Not sure what Trent means about
"giving
> a
> > better guarantee
> > that the same mistake isn't simply made again in future"
> >  -- that CL sets up an
> > architecture where it's always deleted on the UI thread.
> 
> Architecting a set of functions that happen to do a thing isn't the same as
> having
> a type that enforces that that thing must be done when using that type.
> 
> It's unfortunate that we don't have a non-refcounted wrapper that enforces the
> same thing as content::BrowserThread::DeleteOnUIThread. refcounts are a
> tradeoff, but
> not a significant one IMO this isn't high-performance code.
> Also base::BindStateBase is refcounted. Every call to
> base::Bind is effectively adding a refcount around a ShortcutInfo anyway.
> 
> Without DeleteOnUIThread it's still easy to accidentally pass a ShortcutInfo
to
> another thread and have it deleted there. Remembering that you need to
architect
> your functions in a particular way is an unnecessary burden.
> 
> > If you wanted to, you
> > could also add a thread checker to the ShortcutInfo destructor.
> 
> That won't help. The risk is that it *might* get deleted on the wrong thread
> subject to some unpredictable data races that may or may not happen.
> 
> > 
> > Whereas this one just papers over the ownership by having it managed at
> runtime
> > -- very hard to reason about.
> 
> I am an ObjC developer so maybe I have been desensitised to refcounting :p

Heh maybe. The objection to refcounting is not performance. It's that every time
I see refcounted code (i.e., gfx::Image, cough cough), I have no idea under what
conditions (if any) it will be deleted, or on what thread. RefCountedThreadSafe
means we don't really have to reason about what thread something gets deleted
on, but that just means we know less about what's going on. (I tried making
gfx::Image RefCountedThreadSafe and rsesek said "no, instead make sure they are
treated thread-safely".) C++ gives us the tools to reason about ownership and
refcounting and we should use them where possible.