Avoid retaining a reference to ActivityIconLoader in its OnIconReady()

Avoid retaining a reference to ActivityIconLoader in its OnIconReady()

ActivityIconLoader has a non thread safe refcount, and hops to the
other thread with PostTaskAndReplyWithResult.
Though that is safe in the current implementation of PostTaskAndReplyWithResult,
it's fragile and upcoming PostTaskAndReplyWithResult will break it.

This CL removes the refcount bump on the thread hop to avoid a racy
refcount decrement.

Review-Url: https://codereview.chromium.org/2655213004
Cr-Commit-Position: refs/heads/master@{#446332}
Committed: https://chromium.googlesource.com/chromium/src/+/7ef354f5af9e651f60ed079fdbb176b924952e31

[tzik, 2017-01-26 12:40:27]

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

[commit-bot: I haz the power, 2017-01-26 12:40:43]

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

[tzik, 2017-01-26 12:41:25]

tzik@chromium.org changed reviewers:
+ hidehiko@chromium.org

[tzik, 2017-01-26 12:41:25]

PTAL

[tzik, 2017-01-26 13:07:55]

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

[commit-bot: I haz the power, 2017-01-26 13:08:10]

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

[hidehiko, 2017-01-26 13:14:20]

CC: yusukes@.

https://codereview.chromium.org/2655213004/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/activity_icon_loader.cc (right):

https://codereview.chromium.org/2655213004/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/activity_icon_loader.cc:166:
base::Bind(&ActivityIconLoader::ResizeAndEncodeIcons,
(I know it is not your mistake, but ...)

ResizeAndEncodeIcons can be static. So could you mark as static and remove
"this" from here, instead?

thread_checker_ DCHECK looks wrong check, because this instance is created on
the browser thread, while the method should run on a BlockingPool thread, so
could you remove it?

You can keep the field, because we still want to check the thread where other
methods run. So, as you're here, could you add

DCHECK(thread_checker_.CalledOnValidThread()); 

to the beginning of the following methods?
- InvalidateIcons()
- GetActivityIcons()
- OnIconsReady() and
- OnIconsResized().

[commit-bot: I haz the power, 2017-01-26 13:19:00]

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

[commit-bot: I haz the power, 2017-01-26 13:19:01]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[tzik, 2017-01-26 13:19:56]

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

[commit-bot: I haz the power, 2017-01-26 13:20:12]

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

[tzik, 2017-01-26 13:21:12]

https://codereview.chromium.org/2655213004/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/activity_icon_loader.cc (right):

https://codereview.chromium.org/2655213004/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/activity_icon_loader.cc:166:
base::Bind(&ActivityIconLoader::ResizeAndEncodeIcons,
On 2017/01/26 13:14:20, hidehiko wrote:
> (I know it is not your mistake, but ...)
> 
> ResizeAndEncodeIcons can be static. So could you mark as static and remove
> "this" from here, instead?
> 
> thread_checker_ DCHECK looks wrong check, because this instance is created on
> the browser thread, while the method should run on a BlockingPool thread, so
> could you remove it?
> 
> You can keep the field, because we still want to check the thread where other
> methods run. So, as you're here, could you add
> 
> DCHECK(thread_checker_.CalledOnValidThread()); 
> 
> to the beginning of the following methods?
> - InvalidateIcons()
> - GetActivityIcons()
> - OnIconsReady() and
> - OnIconsResized().

Done.

[commit-bot: I haz the power, 2017-01-26 13:46:37]

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

[commit-bot: I haz the power, 2017-01-26 13:46:37]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-01-26 13:54:31]

lgtm

Thanks!

[tzik, 2017-01-26 15:13:29]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-01-26 15:13:47]

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-01-26 15:17:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485443609500810,
"parent_rev": "962af99c3ea7571b042f7f371837eff89fcadeee", "commit_rev":
"7ef354f5af9e651f60ed079fdbb176b924952e31"}

[commit-bot: I haz the power, 2017-01-26 15:18:20]

Description was changed from

==========
Avoid retaining a reference to ActivityIconLoader in its OnIconReady()

ActivityIconLoader has a non thread safe refcount, and hops to the
other thread with PostTaskAndReplyWithResult.
Though that is safe in the current implementation of PostTaskAndReplyWithResult,
it's fragile and upcoming PostTaskAndReplyWithResult will break it.

This CL removes the refcount bump on the thread hop to avoid a racy
refcount decrement.
==========

to

==========
Avoid retaining a reference to ActivityIconLoader in its OnIconReady()

ActivityIconLoader has a non thread safe refcount, and hops to the
other thread with PostTaskAndReplyWithResult.
Though that is safe in the current implementation of PostTaskAndReplyWithResult,
it's fragile and upcoming PostTaskAndReplyWithResult will break it.

This CL removes the refcount bump on the thread hop to avoid a racy
refcount decrement.

Review-Url: https://codereview.chromium.org/2655213004
Cr-Commit-Position: refs/heads/master@{#446332}
Committed:
https://chromium.googlesource.com/chromium/src/+/7ef354f5af9e651f60ed079fdbb1...
==========

[commit-bot: I haz the power, 2017-01-26 15:18:21]

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