Avoid shutdown crash if audio thread is hung.

Avoid shutdown crash if audio thread is hung.

BUG=729494
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2934613002
Cr-Commit-Position: refs/heads/master@{#479969}
Committed: https://chromium.googlesource.com/chromium/src/+/7a4df7ece78ac404d153133e5a80577838ef56f9

[alokp, 2017-06-09 20:05:30]

Description was changed from

==========
Avoid shutdown crash if audio thread is hung.

BUG=729494
==========

to

==========
Avoid shutdown crash if audio thread is hung.

BUG=729494
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[alokp, 2017-06-09 20:05:40]

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

[commit-bot: I haz the power, 2017-06-09 20:06:17]

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-06-09 20:17:15]

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

[commit-bot: I haz the power, 2017-06-09 20:17:16]

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

[alokp, 2017-06-09 20:23:06]

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

[commit-bot: I haz the power, 2017-06-09 20:23:38]

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-06-09 20:46:17]

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

[commit-bot: I haz the power, 2017-06-09 20:46:18]

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

[alokp, 2017-06-12 23:48:47]

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

[commit-bot: I haz the power, 2017-06-12 23:49:27]

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-06-13 00:15:30]

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

[commit-bot: I haz the power, 2017-06-13 00:15:31]

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

[alokp, 2017-06-13 22:53:38]

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

[commit-bot: I haz the power, 2017-06-13 22:54:16]

Dry run: CQ is trying da patch.

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

[alokp, 2017-06-13 23:35:50]

alokp@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[alokp, 2017-06-13 23:35:51]

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
media/audio/audio_manager_base.cc:101: DCHECK_EQ(0, num_output_streams_);
Changing CHECKs in the constructor to DCHECKs since we are intentionally leaking
these streams if audio thread is hung.

[DaleCurtis, 2017-06-13 23:43:15]

Doesn't this leave the streams running and potentially hit use-after-frees? I
think instead you want to skip ~AudioManager, ~AudioSystem entirely?

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
media/audio/audio_manager.cc:224: mutable base::Lock hang_lock_;
Just don't make the method const.

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
media/audio/audio_manager.cc:352: audio_thread_->Stop();
~audio_thread_ will still call stop now. You need to
audio_thread_->ReleaseSoon(audio_thread_) or post it to another thread for
destruction.

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
media/audio/audio_manager_base.cc:101: DCHECK_EQ(0, num_output_streams_);
On 2017/06/13 at 23:35:51, alokp wrote:
> Changing CHECKs in the constructor to DCHECKs since we are intentionally
leaking these streams if audio thread is hung.

I think a better option would be to skip AM::Shutdown(), ~AM and ~AudioSystem if
the audio thread is hung and mark it as a "TODO(olka, grunell): Will be fixed
when audio is its own process."

[commit-bot: I haz the power, 2017-06-14 00:55:02]

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

[commit-bot: I haz the power, 2017-06-14 00:55:03]

Dry run: This issue passed the CQ dry run.

[alokp, 2017-06-14 17:34:33]

alokp@chromium.org changed reviewers:
+ olka@chromium.org

[alokp, 2017-06-14 17:34:35]

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
media/audio/audio_manager.cc:224: mutable base::Lock hang_lock_;
On 2017/06/13 23:43:15, DaleCurtis wrote:
> Just don't make the method const.

Done.

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
media/audio/audio_manager.cc:352: audio_thread_->Stop();
On 2017/06/13 23:43:15, DaleCurtis wrote:
> ~audio_thread_ will still call stop now. You need to
> audio_thread_->ReleaseSoon(audio_thread_) or post it to another thread for
> destruction.

Good catch. I followed your suggestion of leaking the AudioManager instance
which should take care of this.

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/2934613002/diff/60001/media/audio/audio_manag...
media/audio/audio_manager_base.cc:101: DCHECK_EQ(0, num_output_streams_);
On 2017/06/13 23:43:15, DaleCurtis wrote:
> On 2017/06/13 at 23:35:51, alokp wrote:
> > Changing CHECKs in the constructor to DCHECKs since we are intentionally
> leaking these streams if audio thread is hung.
> 
> I think a better option would be to skip AM::Shutdown(), ~AM and ~AudioSystem
if
> the audio thread is hung and mark it as a "TODO(olka, grunell): Will be fixed
> when audio is its own process."

Good idea. Done.

[alokp, 2017-06-14 17:34:44]

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

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

Dry run: CQ is trying da patch.

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

[DaleCurtis, 2017-06-14 19:25:34]

lgtm

[commit-bot: I haz the power, 2017-06-14 19:33:54]

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

[commit-bot: I haz the power, 2017-06-14 19:33:55]

Dry run: This issue passed the CQ dry run.

[alokp, 2017-06-14 20:02:19]

alokp@chromium.org changed reviewers:
+ pfeldman@chromium.org

[alokp, 2017-06-14 20:02:20]

pfeldman: content/

[pfeldman, 2017-06-14 22:38:31]

https://codereview.chromium.org/2934613002/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2934613002/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.cc:86: return audio_thread_status_ == THREAD_HUNG;
We should not sync UI thread with potentially stalled audio one.

https://codereview.chromium.org/2934613002/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.cc:344: if (GetHelper()->IsAudioThreadHung())
Are you suggesting that upon stopping the audio thread caller is joined and that
causes caller thread to hang? In this case you can try stopping this thread via
anther IO-type thread not sensitive to the stall (FILE?).

Also, what if you haven't yet updated status to HUNG? For example, failed_pings_
== 2. So we are hanging, but we don't know about it yet.

[alokp, 2017-06-14 23:56:19]

https://codereview.chromium.org/2934613002/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2934613002/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.cc:86: return audio_thread_status_ == THREAD_HUNG;
On 2017/06/14 22:38:30, pfeldman wrote:
> We should not sync UI thread with potentially stalled audio one.

This function is only called during shutdown. Are you worried that audio-thread
would have acquired |hang_lock_| before stalling?

https://codereview.chromium.org/2934613002/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.cc:344: if (GetHelper()->IsAudioThreadHung())
On 2017/06/14 22:38:31, pfeldman wrote:
> Are you suggesting that upon stopping the audio thread caller is joined and
that
> causes caller thread to hang?

Yes - exactly.

> In this case you can try stopping this thread via
> anther IO-type thread not sensitive to the stall (FILE?).

AudioManager::Shutdown is currently called *after* all threads have been stopped
(see BrowserMainLoop::ShutdownThreadsAndCleanUp). So it is not possible to use
any other thread with current design.

That said, lifetime of audio thread is only tied to IO thread. Audio thread must
be stopped *after* joining IO thread since the IO thread posts tasks to the
audio thread that must run to achieve clean shutdown. I am not sure if it is
worth the complexity though since media team is actively working on moving the
audio service into a separate process. Dale?

> Also, what if you haven't yet updated status to HUNG? For example,
failed_pings_
> == 2. So we are hanging, but we don't know about it yet.

Yeah this is best effort. There is no guarantee that audio thread will not stall
between now and the time it is actually joined.

[o1ka, 2017-06-15 16:01:17]

lgtm

[pfeldman, 2017-06-16 00:18:07]

lgtm

[alokp, 2017-06-16 01:30:41]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2017-06-16 01:31:18]

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-06-16 02:31:18]

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

[commit-bot: I haz the power, 2017-06-16 02:31:19]

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...)

[alokp, 2017-06-16 03:43:34]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2017-06-16 03:43:55]

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-06-16 06:35:11]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497584614998810,
"parent_rev": "874db1d83fc2851f19a2687ade5ad2ddf5df92bb", "commit_rev":
"7a4df7ece78ac404d153133e5a80577838ef56f9"}

[commit-bot: I haz the power, 2017-06-16 06:35:30]

Description was changed from

==========
Avoid shutdown crash if audio thread is hung.

BUG=729494
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Avoid shutdown crash if audio thread is hung.

BUG=729494
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2934613002
Cr-Commit-Position: refs/heads/master@{#479969}
Committed:
https://chromium.googlesource.com/chromium/src/+/7a4df7ece78ac404d153133e5a80...
==========

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

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