Adds locks for CommandBufferProxyImpl invalidation

Adds locks for CommandBufferProxyImpl invalidation

This CL tries to fix the race condition between the destruction of
CommandBufferProxyImpl and GpuVideo{En,De}codeAcceleratorHost. 
CommandBufferProxyImpl is destroyed on main_thread whereas the other
is destroyed on media thread.
- OnWillDeleteImpl() is called on main thread and the thread check 
is wrong.
- Added lock around |impl_| use.
- Post a task on media thread for OnChannelError().

BUG=627295, 674674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2627173002
Cr-Commit-Position: refs/heads/master@{#443695}
Committed: https://chromium.googlesource.com/chromium/src/+/ddd5cb6aad0421c0fa7f82207531ada865bfe7f0

[emircan, 2017-01-12 00:14:32]

Description was changed from

==========
Fix multiple thread access.

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

to

==========
Fix multiple thread access.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[emircan, 2017-01-12 00:14:36]

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

[commit-bot: I haz the power, 2017-01-12 00:15:37]

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

[emircan, 2017-01-12 00:29:42]

Description was changed from

==========
Fix multiple thread access.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Adds locks for CommandBufferProxyImpl invalidation

This CL tries to fix the race condition between the destruction of
CommandBufferProxyImpl and GpuVideo{En,De}codeAcceleratorHost. 
CommandBufferProxyImpl is destroyed on main_thread whereas the other
is destroyed on media thread.
- OnWillDeleteImpl() is called on media thread and the thread check 
is wrong.
- Added lock around |impl_| use.
- Post a task on media thread for OnChannelError().

BUG=627295,674674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[emircan, 2017-01-12 00:29:42]

emircan@chromium.org changed reviewers:
+ sandersd@chromium.org

[emircan, 2017-01-12 00:31:17]

sandersd@, I believe crbug.com/674674 is due to the same underlying issue you
described on https://bugs.chromium.org/p/chromium/issues/detail?id=627295#c2. I
tried to fix it the way you described, PTAL.

[commit-bot: I haz the power, 2017-01-12 01:45:18]

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

[commit-bot: I haz the power, 2017-01-12 01:45:19]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2017-01-12 19:06:53]

lgtm

[emircan, 2017-01-12 19:07:53]

emircan@chromium.org changed reviewers:
+ danakj@chromium.org

[emircan, 2017-01-12 19:07:53]

Thanks. 

danakj@ can you PTAL as well based on our discussion yesterday?

[emircan, 2017-01-13 18:21:25]

Description was changed from

==========
Adds locks for CommandBufferProxyImpl invalidation

This CL tries to fix the race condition between the destruction of
CommandBufferProxyImpl and GpuVideo{En,De}codeAcceleratorHost. 
CommandBufferProxyImpl is destroyed on main_thread whereas the other
is destroyed on media thread.
- OnWillDeleteImpl() is called on media thread and the thread check 
is wrong.
- Added lock around |impl_| use.
- Post a task on media thread for OnChannelError().

BUG=627295,674674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Adds locks for CommandBufferProxyImpl invalidation

This CL tries to fix the race condition between the destruction of
CommandBufferProxyImpl and GpuVideo{En,De}codeAcceleratorHost. 
CommandBufferProxyImpl is destroyed on main_thread whereas the other
is destroyed on media thread.
- OnWillDeleteImpl() is called on main thread and the thread check 
is wrong.
- Added lock around |impl_| use.
- Post a task on media thread for OnChannelError().

BUG=627295,674674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2017-01-13 18:27:51]

LGTM

https://codereview.chromium.org/2627173002/diff/1/media/gpu/ipc/client/gpu_vi...
File media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/2627173002/diff/1/media/gpu/ipc/client/gpu_vi...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:30: channel_error_cb_
= media::BindToCurrentLoop(
Just store a task runner and bind/post to it from OnWillDeleteImpl? That would
be a lot easier to follow/less indirection to what is going on.

https://codereview.chromium.org/2627173002/diff/1/media/gpu/ipc/client/gpu_vi...
File media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc (right):

https://codereview.chromium.org/2627173002/diff/1/media/gpu/ipc/client/gpu_vi...
media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc:30: channel_error_cb_
= media::BindToCurrentLoop(
same

[emircan, 2017-01-13 18:56:19]

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

[commit-bot: I haz the power, 2017-01-13 18:57:09]

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

[emircan, 2017-01-13 19:07:34]

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

[emircan, 2017-01-13 19:07:43]

Patchset #2 (id:20001) has been deleted

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

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

[emircan, 2017-01-13 19:08:14]

https://codereview.chromium.org/2627173002/diff/1/media/gpu/ipc/client/gpu_vi...
File media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/2627173002/diff/1/media/gpu/ipc/client/gpu_vi...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:30: channel_error_cb_
= media::BindToCurrentLoop(
On 2017/01/13 18:27:50, danakj (slow) wrote:
> Just store a task runner and bind/post to it from OnWillDeleteImpl? That would
> be a lot easier to follow/less indirection to what is going on.

Done.

https://codereview.chromium.org/2627173002/diff/1/media/gpu/ipc/client/gpu_vi...
File media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc (right):

https://codereview.chromium.org/2627173002/diff/1/media/gpu/ipc/client/gpu_vi...
media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc:30: channel_error_cb_
= media::BindToCurrentLoop(
On 2017/01/13 18:27:50, danakj (slow) wrote:
> same

Done.

[emircan, 2017-01-13 19:25:46]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2017-01-13 19:25:46]

The CQ bit was checked by emircan@chromium.org

[emircan, 2017-01-13 19:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2627173002/#ps40001
(title: " ")

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

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

[danakj, 2017-01-13 19:32:04]

cool! thanks LGTM even more

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
File media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:15: #include
"media/base/bind_to_current_loop.h"
unused then

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
File media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc (right):

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc:11: #include
"media/base/bind_to_current_loop.h"
same

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc:26:
task_runner_(base::ThreadTaskRunnerHandle::Get()),
can you give these task runners a name that describes their thread?

[emircan, 2017-01-13 19:37:30]

The CQ bit was unchecked by emircan@chromium.org

[emircan, 2017-01-13 19:40:59]

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

[emircan, 2017-01-13 19:41:09]

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
File media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_decode_accelerator_host.cc:15: #include
"media/base/bind_to_current_loop.h"
On 2017/01/13 19:32:04, danakj (slow) wrote:
> unused then

Sorry i missed out. Removing.

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
File media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc (right):

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc:11: #include
"media/base/bind_to_current_loop.h"
On 2017/01/13 19:32:04, danakj (slow) wrote:
> same

Done.

https://codereview.chromium.org/2627173002/diff/40001/media/gpu/ipc/client/gp...
media/gpu/ipc/client/gpu_video_encode_accelerator_host.cc:26:
task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2017/01/13 19:32:04, danakj (slow) wrote:
> can you give these task runners a name that describes their thread?

Done.

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

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

[emircan, 2017-01-13 19:41:45]

Patchset #3 (id:60001) has been deleted

[danakj, 2017-01-13 19:42:03]

Super great, thanks.

[emircan, 2017-01-13 19:42:16]

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

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

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

[emircan, 2017-01-13 20:07:14]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-01-13 20:07:33]

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

[emircan, 2017-01-13 20:09:36]

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

[commit-bot: I haz the power, 2017-01-13 20:09:49]

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-01-13 21:58:07]

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

[commit-bot: I haz the power, 2017-01-13 21:58:07]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-01-13 22:10:14]

The CQ bit was checked by emircan@chromium.org

[emircan, 2017-01-13 22:10:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2627173002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2017-01-13 22:11:10]

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-13 22:17:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484345414025040,
"parent_rev": "d5a7a73975d123369dab9784b2897467a31b0ab8", "commit_rev":
"ddd5cb6aad0421c0fa7f82207531ada865bfe7f0"}

[commit-bot: I haz the power, 2017-01-13 22:17:50]

Description was changed from

==========
Adds locks for CommandBufferProxyImpl invalidation

This CL tries to fix the race condition between the destruction of
CommandBufferProxyImpl and GpuVideo{En,De}codeAcceleratorHost. 
CommandBufferProxyImpl is destroyed on main_thread whereas the other
is destroyed on media thread.
- OnWillDeleteImpl() is called on main thread and the thread check 
is wrong.
- Added lock around |impl_| use.
- Post a task on media thread for OnChannelError().

BUG=627295,674674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Adds locks for CommandBufferProxyImpl invalidation

This CL tries to fix the race condition between the destruction of
CommandBufferProxyImpl and GpuVideo{En,De}codeAcceleratorHost. 
CommandBufferProxyImpl is destroyed on main_thread whereas the other
is destroyed on media thread.
- OnWillDeleteImpl() is called on main thread and the thread check 
is wrong.
- Added lock around |impl_| use.
- Post a task on media thread for OnChannelError().

BUG=627295,674674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2627173002
Cr-Commit-Position: refs/heads/master@{#443695}
Committed:
https://chromium.googlesource.com/chromium/src/+/ddd5cb6aad0421c0fa7f82207531...
==========

[commit-bot: I haz the power, 2017-01-13 22:17:51]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ddd5cb6aad0421c0fa7f82207531...