Fix a data race on the ref count of AddToHomescreenDataFetcher

Fix a data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.

BUG=688072
Review-Url: https://codereview.chromium.org/2691943010
Cr-Commit-Position: refs/heads/master@{#451241}
Committed: https://chromium.googlesource.com/chromium/src/+/99f46d6b1a2e559bf0f4f60fe59c2c0690e94ba8

[tzik, 2017-02-16 13:00:05]

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

[commit-bot: I haz the power, 2017-02-16 13:00:23]

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-16 13:09:37]

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

[commit-bot: I haz the power, 2017-02-16 13:09:50]

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-16 13:19:46]

Description was changed from

==========
Fix data race on the ref count of AddToHomescreenDataFetcher
==========

to

==========
Fix data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.
==========

[tzik, 2017-02-16 13:22:50]

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

[tzik, 2017-02-16 13:22:51]

PTAL

[tzik, 2017-02-16 13:23:07]

Description was changed from

==========
Fix data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.
==========

to

==========
Fix a data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.
==========

[pkotwicz, 2017-02-16 14:27:48]

pkotwicz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[pkotwicz, 2017-02-16 14:27:49]

I am not a reviewer. Your CL looks good to me though

Can you include a bug # in your CL description? Did you find the data race as
part of investigating a different bug? Did you find it via a script? Was the
data race causing crashes?

[commit-bot: I haz the power, 2017-02-16 14:56:22]

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

[commit-bot: I haz the power, 2017-02-16 14:56:23]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-02-16 16:29:18]

Description was changed from

==========
Fix a data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.
==========

to

==========
Fix a data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.

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

[tzik, 2017-02-16 17:49:51]

On 2017/02/16 14:27:49, pkotwicz wrote:
> I am not a reviewer. Your CL looks good to me though
> 
> Can you include a bug # in your CL description? Did you find the data race as
> part of investigating a different bug? Did you find it via a script? Was the
> data race causing crashes?

Added BUG= line to the description.
I found this while I scanned cross thread usage of RefCount by adding a DCHECK
to its manipulation: https://codereview.chromium.org/2666423002/

[gone, 2017-02-16 18:49:49]

dfalcantara@chromium.org changed reviewers:
+ dominickn@chromium.org

[gone, 2017-02-16 18:49:49]

Sending to dominickn@ because he's more familiar with it.

[dominickn, 2017-02-16 23:24:45]

https://codereview.chromium.org/2691943010/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2691943010/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:11: #include
"base/location.h"
Nit: #include "base/task_runner_util.h" for base::PostTaskAndReplyWithResult

https://codereview.chromium.org/2691943010/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:297:
base::Unretained(this), bitmap_result),
Is it safe to use base::Unretained here? AddToHomescreenDataFetcher can be
destroyed by the user dismissing the add to homescreen dialog in Java, which
destroys the AddToHomescreenManager object which carries the owning
scoped_refptr to this.

I think it's possible for that to happen whilst the image processing is
happening on the background thread, leading to a method being called on a
released object when the reply is called.

Or will the destruction of the task runner in this object also ensure that the
task is safely canceled if this object is gone (though it's a scoped_refptr so
probably it won't be destroyed and the task still run)?

[tzik, 2017-02-17 04:57:48]

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

[tzik, 2017-02-17 04:58:20]

https://codereview.chromium.org/2691943010/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2691943010/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:11: #include
"base/location.h"
On 2017/02/16 23:24:45, dominickn wrote:
> Nit: #include "base/task_runner_util.h" for base::PostTaskAndReplyWithResult

Done.

https://codereview.chromium.org/2691943010/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:297:
base::Unretained(this), bitmap_result),
On 2017/02/16 23:24:45, dominickn wrote:
> Is it safe to use base::Unretained here? AddToHomescreenDataFetcher can be
> destroyed by the user dismissing the add to homescreen dialog in Java, which
> destroys the AddToHomescreenManager object which carries the owning
> scoped_refptr to this.

Yes, it's safe, since a reference is implicitly retained by the Reply part of
Callback.
Added RetainedRef annotation here to make it more readable.

> 
> I think it's possible for that to happen whilst the image processing is
> happening on the background thread, leading to a method being called on a
> released object when the reply is called.
> 
> Or will the destruction of the task runner in this object also ensure that the
> task is safely canceled if this object is gone (though it's a scoped_refptr so
> probably it won't be destroyed and the task still run)?

[commit-bot: I haz the power, 2017-02-17 04:58:26]

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

[dominickn, 2017-02-17 05:00:37]

Thanks. lgtm

[tzik, 2017-02-17 05:38:06]

The CQ bit was unchecked by tzik@chromium.org

[tzik, 2017-02-17 05:38:15]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2017-02-17 05:38:56]

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-17 06:16:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487309895798270,
"parent_rev": "6bd4c19f45d6e2bba69f9668516a829f39ca9a1f", "commit_rev":
"99f46d6b1a2e559bf0f4f60fe59c2c0690e94ba8"}

[commit-bot: I haz the power, 2017-02-17 06:17:35]

Description was changed from

==========
Fix a data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.

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

to

==========
Fix a data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.

BUG=688072

Review-Url: https://codereview.chromium.org/2691943010
Cr-Commit-Position: refs/heads/master@{#451241}
Committed:
https://chromium.googlesource.com/chromium/src/+/99f46d6b1a2e559bf0f4f60fe59c...
==========

[commit-bot: I haz the power, 2017-02-17 06:17:37]

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