Destroy bound callback to BindToCurrentLoop on the target task runner (2)

Destroy bound callback to BindToCurrentLoop on the target task runner

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.

BUG=688072
Review-Url: https://codereview.chromium.org/2712603002
Cr-Commit-Position: refs/heads/master@{#454212}
Committed: https://chromium.googlesource.com/chromium/src/+/2b817313110d51783e803320cd2d102c51166742

[tzik, 2017-02-22 02:30:17]

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

[commit-bot: I haz the power, 2017-02-22 02:31:33]

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-22 04:36:00]

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

[commit-bot: I haz the power, 2017-02-22 04:36:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[tzik, 2017-02-22 05:51:28]

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

[commit-bot: I haz the power, 2017-02-22 05:52:55]

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-22 06:15:10]

Description was changed from

==========
Overhaul BindToCurrentLoop

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

to

==========
Destroy bound callback by BindToCurrentLoop on the target task runner

123456789012345678901234567890123456789012345678901234567890123456789012

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the race.
==========

[tzik, 2017-02-22 06:15:32]

Description was changed from

==========
Destroy bound callback by BindToCurrentLoop on the target task runner

123456789012345678901234567890123456789012345678901234567890123456789012

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the race.
==========

to

==========
Destroy bound callback by BindToCurrentLoop on the target task runner

123456789012345678901234567890123456789012345678901234567890123456789012

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.
==========

[tzik, 2017-02-22 06:17:01]

Description was changed from

==========
Destroy bound callback by BindToCurrentLoop on the target task runner

123456789012345678901234567890123456789012345678901234567890123456789012

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.
==========

to

==========
Destroy bound callback by BindToCurrentLoop on the target task runner

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.

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

[tzik, 2017-02-22 06:17:44]

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

[tzik, 2017-02-22 06:17:44]

PTAL

[tzik, 2017-02-22 06:18:03]

Description was changed from

==========
Destroy bound callback by BindToCurrentLoop on the target task runner

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.

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

to

==========
Destroy bound callback to BindToCurrentLoop on the target task runner

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.

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

[commit-bot: I haz the power, 2017-02-22 07:35:10]

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

[commit-bot: I haz the power, 2017-02-22 07:35:11]

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

[tzik, 2017-02-22 22:21:51]

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

[commit-bot: I haz the power, 2017-02-22 22:23:03]

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-22 23:59:13]

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

[commit-bot: I haz the power, 2017-02-22 23:59:14]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-02-23 04:52:05]

xhwang@chromium.org changed reviewers:
+ danakj@chromium.org, johnme@chromium.org

[xhwang, 2017-02-23 04:52:06]

Add johnme and danakj, who were involved in similar discussion before. You can
see the previous CL at https://chromiumcodereview.appspot.com/1082113004/

We also talked about moving BindToCurrentLoop to base/ at
https://bugs.chromium.org/p/chromium/issues/detail?id=432381

I haven't take a detailed look yet, which I'll do tonight or tomorrow.

[xhwang, 2017-02-23 05:45:49]

The change looks good to me. Can you add a unit test for the case where the
callback never runs and is destroyed?

[danakj, 2017-02-23 21:29:06]

https://codereview.chromium.org/2712603002/diff/20001/media/base/bind_to_curr...
File media/base/bind_to_current_loop.h (right):

https://codereview.chromium.org/2712603002/diff/20001/media/base/bind_to_curr...
media/base/bind_to_current_loop.h:55: std::move(callback_)));
Isn't this thread-racey? The last reference in the return from Bind may be
destroyed in ~TrampolineHelper or in ClearCallbackOnTargetTaskRunner, which are
on different threads. You should used base::Passed and take the callback by
value in the Clear method I think?

[tzik, 2017-02-27 06:27:01]

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

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

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 06:28:04]

https://codereview.chromium.org/2712603002/diff/20001/media/base/bind_to_curr...
File media/base/bind_to_current_loop.h (right):

https://codereview.chromium.org/2712603002/diff/20001/media/base/bind_to_curr...
media/base/bind_to_current_loop.h:55: std::move(callback_)));
On 2017/02/23 21:29:06, danakj wrote:
> Isn't this thread-racey? The last reference in the return from Bind may be
> destroyed in ~TrampolineHelper or in ClearCallbackOnTargetTaskRunner, which
are
> on different threads. You should used base::Passed and take the callback by
> value in the Clear method I think?

Ah, good point. Right, it was still racy. Updated!

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

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

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

Dry run: This issue passed the CQ dry run.

[danakj, 2017-02-28 18:28:04]

https://codereview.chromium.org/2712603002/diff/40001/media/base/bind_to_curr...
File media/base/bind_to_current_loop.h (right):

https://codereview.chromium.org/2712603002/diff/40001/media/base/bind_to_curr...
media/base/bind_to_current_loop.h:59: static void
ClearCallbackOnTargetTaskRunner(const CallbackType&) {}
This should receive by value so the Callback can move() it here right? Else it's
still left inside the callback?

[tzik, 2017-03-01 05:49:59]

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:50:13]

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:51:49]

https://codereview.chromium.org/2712603002/diff/40001/media/base/bind_to_curr...
File media/base/bind_to_current_loop.h (right):

https://codereview.chromium.org/2712603002/diff/40001/media/base/bind_to_curr...
media/base/bind_to_current_loop.h:59: static void
ClearCallbackOnTargetTaskRunner(const CallbackType&) {}
On 2017/02/28 18:28:04, danakj_OOO wrote:
> This should receive by value so the Callback can move() it here right? Else
it's
> still left inside the callback?

Hm, yes, this should be passed by value, though base::Passed() passes it by
value and destroy it anyway. Updated.

[commit-bot: I haz the power, 2017-03-01 07:24:59]

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

[commit-bot: I haz the power, 2017-03-01 07:25:00]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-03-01 17:10:42]

LGTM

https://codereview.chromium.org/2712603002/diff/60001/media/base/bind_to_curr...
File media/base/bind_to_current_loop.h (right):

https://codereview.chromium.org/2712603002/diff/60001/media/base/bind_to_curr...
media/base/bind_to_current_loop.h:85: base::Callback<T>
BindToCurrentLoop(base::Callback<T> cb) {
should this be marked inline?

[xhwang, 2017-03-01 17:23:23]

lgtm, thanks!

[tzik, 2017-03-02 06:49:28]

https://codereview.chromium.org/2712603002/diff/60001/media/base/bind_to_curr...
File media/base/bind_to_current_loop.h (right):

https://codereview.chromium.org/2712603002/diff/60001/media/base/bind_to_curr...
media/base/bind_to_current_loop.h:85: base::Callback<T>
BindToCurrentLoop(base::Callback<T> cb) {
On 2017/03/01 17:10:42, danakj wrote:
> should this be marked inline?

Done.

[tzik, 2017-03-02 06:49:38]

The CQ bit was checked by tzik@chromium.org

[tzik, 2017-03-02 06:49:38]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2712603002/#ps80001
(title: "+inline")

[commit-bot: I haz the power, 2017-03-02 06:50:29]

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-03-02 08:29:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488437378578820,
"parent_rev": "460361ba8ca8615d1794caa3e6dc517634798b4f", "commit_rev":
"2b817313110d51783e803320cd2d102c51166742"}

[commit-bot: I haz the power, 2017-03-02 08:30:00]

Description was changed from

==========
Destroy bound callback to BindToCurrentLoop on the target task runner

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.

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

to

==========
Destroy bound callback to BindToCurrentLoop on the target task runner

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.

BUG=688072

Review-Url: https://codereview.chromium.org/2712603002
Cr-Commit-Position: refs/heads/master@{#454212}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b817313110d51783e803320cd2d...
==========

[commit-bot: I haz the power, 2017-03-02 08:30:01]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2b817313110d51783e803320cd2d...