Ensures that audio tasks cannot run after AudioManager is deleted.

Ensures that audio tasks cannot run after AudioManager is deleted.

AudioManager is now deleted on the main thread instead of the audio
thread. It is Shutdown on the audio thread which allows it to release
resources on the correct thread. Audio thread is then stopped before
audio manager is deleted so that all tasks in the audio thread queue
runs and no more tasks can be posted.

BUG=705455
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/2784433002
Cr-Commit-Position: refs/heads/master@{#471558}
Committed: https://chromium.googlesource.com/chromium/src/+/2cbd4fcab300af5aa4ef76752673901d58da0f24

[alokp, 2017-03-28 00:15:35]

Description was changed from

==========
Ensures that audio tasks cannot run after AudioManager is deleted.

BUG=705455
==========

to

==========
Ensures that audio tasks cannot run after AudioManager is deleted.

BUG=705455
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-03-28 00:15:43]

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

[commit-bot: I haz the power, 2017-03-28 00:16:04]

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-03-28 00:26:22]

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

[commit-bot: I haz the power, 2017-03-28 00:26:23]

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_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[alokp, 2017-03-28 04:04:44]

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

[commit-bot: I haz the power, 2017-03-28 04:05:09]

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-03-28 04:14:39]

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

[commit-bot: I haz the power, 2017-03-28 04:14:40]

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

[alokp, 2017-03-28 05:32:52]

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

[commit-bot: I haz the power, 2017-03-28 05:33: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-03-28 05:44:09]

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

[commit-bot: I haz the power, 2017-03-28 05:44:10]

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

[alokp, 2017-03-28 17:49:44]

Description was changed from

==========
Ensures that audio tasks cannot run after AudioManager is deleted.

BUG=705455
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

==========
Ensures that audio tasks cannot run after AudioManager is deleted.

AudioManager is now deleted on the main thread instead of the audio
thread. It is Shutdown on the audio thread which allows it to release
resources on the correct thread. Audio thread is then stopped before
audio manager is deleted so that all tasks in the audio thread queue
runs and no more tasks can be posted.

BUG=705455
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-03-28 17:53:00]

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

[alokp, 2017-03-28 17:53:00]

Dale: I still need to fix redness on various bots but I think this is ready for
first round of reviews.

The patch is not as big as it looks. The changes that need to be reviewed
carefully is in browser_main_loop.*. Most other changes are mechanical.

[alokp, 2017-03-28 17:53:15]

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

[commit-bot: I haz the power, 2017-03-28 17:53:46]

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

[alokp, 2017-03-28 17:56:33]

Another important thing to note: It is still possible for tasks to be posted
after the AudioManager::Shutdown task. This patch only fixes the risk of
use-after-free.

Is use-after-shutdown a problem that we need to prevent?

[commit-bot: I haz the power, 2017-03-28 18:02:21]

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

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

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

[DaleCurtis, 2017-03-28 18:12:28]

Overall lg, will stamp once tests are passing.

[o1ka, 2017-03-28 18:32:23]

A couple of questions

https://codereview.chromium.org/2784433002/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2784433002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1820: audio_thread_.reset();
Is there a guarantee that all the queued tasks will run? (Sorry cannot look it
up now - I'm on the go)

https://codereview.chromium.org/2784433002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1823: audio_manager_.reset();
Looks like we need to reset AudioSystem before this?

[alokp, 2017-03-28 18:50:43]

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

[commit-bot: I haz the power, 2017-03-28 18:51:15]

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-03-28 19:02:12]

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

[commit-bot: I haz the power, 2017-03-28 19:02:13]

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

[alokp, 2017-03-28 19:38:33]

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

[commit-bot: I haz the power, 2017-03-28 19:39:05]

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-03-28 19:47:23]

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

[commit-bot: I haz the power, 2017-03-28 19:47:24]

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

[o1ka, 2017-03-29 09:28:32]

https://codereview.chromium.org/2784433002/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2784433002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1820: audio_thread_.reset();
On 2017/03/28 18:32:23, o1ka wrote:
> Is there a guarantee that all the queued tasks will run? (Sorry cannot look it
> up now - I'm on the go)

Never mind - I checked it now, base::Thread will do it :)

[alokp, 2017-03-29 17:45:29]

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

[commit-bot: I haz the power, 2017-03-29 17:46:02]

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-03-29 18:13:51]

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

[commit-bot: I haz the power, 2017-03-29 18:13:52]

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

[alokp, 2017-04-24 23:13:04]

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

[commit-bot: I haz the power, 2017-04-24 23:13:39]

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-04-24 23:18:57]

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

[commit-bot: I haz the power, 2017-04-24 23:18:58]

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...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[alokp, 2017-04-25 21:47:53]

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

[commit-bot: I haz the power, 2017-04-25 21:48:41]

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-04-25 22:05:06]

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

[commit-bot: I haz the power, 2017-04-25 22:05:07]

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

[alokp, 2017-04-26 17:56:34]

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

[commit-bot: I haz the power, 2017-04-26 17:57:32]

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-04-26 18:13:33]

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

[commit-bot: I haz the power, 2017-04-26 18:13:34]

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

[alokp, 2017-04-26 18:59:07]

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

[commit-bot: I haz the power, 2017-04-26 19:00:11]

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-04-26 19:15:59]

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

[commit-bot: I haz the power, 2017-04-26 19:16:00]

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

[alokp, 2017-04-26 20:55:00]

Dale: I have a question for you regarding fallback to Alsa backend. Thanks!

https://codereview.chromium.org/2784433002/diff/270001/media/audio/linux/audi...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/2784433002/diff/270001/media/audio/linux/audi...
media/audio/linux/audio_manager_linux.cc:47: auto manager =
base::MakeUnique<AudioManagerPulse>(std::move(audio_thread),
Dale: Is there an alternate way to check if pulseaudio is supported? If I need
to support this fallback to Alsa, we would need to pass |audio_thread| as raw
pointer. Any suggestions?

[DaleCurtis, 2017-04-26 20:56:35]

https://codereview.chromium.org/2784433002/diff/270001/media/audio/linux/audi...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/2784433002/diff/270001/media/audio/linux/audi...
media/audio/linux/audio_manager_linux.cc:47: auto manager =
base::MakeUnique<AudioManagerPulse>(std::move(audio_thread),
On 2017/04/26 at 20:55:00, alokp wrote:
> Dale: Is there an alternate way to check if pulseaudio is supported? If I need
to support this fallback to Alsa, we would need to pass |audio_thread| as raw
pointer. Any suggestions?

No, but there's no reason we can't extract Init() to some static function and
use that without passing the AM thread in.

[alokp, 2017-04-26 21:02:50]

https://codereview.chromium.org/2784433002/diff/270001/media/audio/linux/audi...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/2784433002/diff/270001/media/audio/linux/audi...
media/audio/linux/audio_manager_linux.cc:47: auto manager =
base::MakeUnique<AudioManagerPulse>(std::move(audio_thread),
On 2017/04/26 20:56:35, DaleCurtis wrote:
> On 2017/04/26 at 20:55:00, alokp wrote:
> > Dale: Is there an alternate way to check if pulseaudio is supported? If I
need
> to support this fallback to Alsa, we would need to pass |audio_thread| as raw
> pointer. Any suggestions?
> 
> No, but there's no reason we can't extract Init() to some static function and
> use that without passing the AM thread in.

Are you suggesting that the static Init return pa_threaded_mainloop and
pa_context that gets fed into AudioManagerPulse constructor? That would work.

[DaleCurtis, 2017-04-26 21:07:09]

https://codereview.chromium.org/2784433002/diff/270001/media/audio/linux/audi...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/2784433002/diff/270001/media/audio/linux/audi...
media/audio/linux/audio_manager_linux.cc:47: auto manager =
base::MakeUnique<AudioManagerPulse>(std::move(audio_thread),
On 2017/04/26 at 21:02:50, alokp wrote:
> On 2017/04/26 20:56:35, DaleCurtis wrote:
> > On 2017/04/26 at 20:55:00, alokp wrote:
> > > Dale: Is there an alternate way to check if pulseaudio is supported? If I
need
> > to support this fallback to Alsa, we would need to pass |audio_thread| as
raw
> > pointer. Any suggestions?
> > 
> > No, but there's no reason we can't extract Init() to some static function
and
> > use that without passing the AM thread in.
> 
> Are you suggesting that the static Init return pa_threaded_mainloop and
pa_context that gets fed into AudioManagerPulse constructor? That would work.

Yup.

[alokp, 2017-04-26 23:15:35]

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

[commit-bot: I haz the power, 2017-04-26 23:16:39]

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-04-26 23:36:41]

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

[commit-bot: I haz the power, 2017-04-26 23:36:42]

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

[alokp, 2017-04-27 18:46:02]

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

[commit-bot: I haz the power, 2017-04-27 18:46: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-04-27 19:05:01]

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

[commit-bot: I haz the power, 2017-04-27 19:05:02]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[alokp, 2017-04-27 20:24:22]

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

[commit-bot: I haz the power, 2017-04-27 20:24:48]

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-04-27 20:46:23]

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

[commit-bot: I haz the power, 2017-04-27 20:46:24]

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

[alokp, 2017-04-27 22:44:37]

This is ready for review.

I would suggest start reviewing the changes in audio_manager*.[h,cc] and
audio_thread*.[h,cc]. Rest of the changes are mechanical.

Moving the ownership of AudioThread into AudioManager did simplify the lifetime
issues quite a bit. Now we just need to ensure that we call
AudioManager::Shutdown before destroying AudioSystem or AudioManager.

[alokp, 2017-04-27 22:44:45]

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

[commit-bot: I haz the power, 2017-04-27 22:45:47]

Dry run: CQ is trying da patch.

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

[DaleCurtis, 2017-04-27 23:28:54]

Nice work, lgtm -- I like the simplified ownership model.

[commit-bot: I haz the power, 2017-04-28 01:37:16]

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

[commit-bot: I haz the power, 2017-04-28 01:37:17]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[o1ka, 2017-04-28 13:24:20]

Great simplification!

https://codereview.chromium.org/2784433002/diff/390001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/content/browser/browse...
content/browser/browser_main_loop.cc:1771: audio_system_.reset();
Audio thread can have AudioSystem operations posted to it, and now they refer to
an invalid pointer.

Should we fix it according to the decision in the mail thread?

1. AudioManager::Shutdown(): This shuts down the AudioManager and stops the
audio thread.
2. delete AudioSystem
3. delete AudioThread
4. delete AudioManager

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:348: audio_thread_->Stop();
Have you verified that hang monitor (l.308) will work correctly after this
change?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:349: audio_thread_.reset();
I believe we should only Stop() here and use another way to DCHEK if
|audio_thread_| is stopped.

See my comment to BrowserMainLoop::DestroyAudioManager()

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:42: virtual ~AudioManager();
Could you specify that Shutdown() is mandatory before destruction?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:191: return audio_thread_->GetTaskRunner();
Basing on AudioManager::Shutdown() |audio_thread_| can be nullptr.
Basing on AudioThreadImpl::Stop() it can return nullptr now.

DCHECK(audio_thread_->GetTaskRunner())?
And how do we make sure it's never called after Shutdown()?
Should we re-think Shutdown() so that AudioManger remains in a valid state after
it?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:199: return audio_thread_->GetWorkerTaskRunner();
Same here

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
File media/audio/audio_system_impl_unittest.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:57:
base::MakeUnique<TestAudioThread>());
Can we have a bool parameter for AudioThreadImpl constructor instead?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:79: ~AudioSystemImplTest() override {
audio_manager_->Shutdown(); }
Note that in this test the destruction sequence is different from the one in the
mail loop: Shutdown, AudioSystem deletion, AudioManager deletion.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
File media/audio/audio_thread.h (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
media/audio/audio_thread.h:23: // Both - GetTaskRunner and GetWorkerTaskRunner -
will return null after this
Should AudioManager check for null?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
File media/audio/audio_thread_impl.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
media/audio/audio_thread_impl.cc:31: task_runner_ = nullptr;
Could you add explanations of how Stop() works in Mac case? Should we DCHECK
that on Mac Stop() is called on task_runner_?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
media/audio/audio_thread_impl.cc:31: task_runner_ = nullptr;
This does not guarantee that task_runner is deleted, right - since it's
ref-counted, and some clients keep a ref ptr to it.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/test_audio...
File media/audio/test_audio_thread.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/test_audio...
media/audio/test_audio_thread.cc:13: :
task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
The purpose of this class needs to be explained in the header. Otherwise it's
too tempting to use it for all tests, but some cases do require a separate audio
thread to be tested properly.

Actually, I would prefer to avoid having this test class. Can we pass "use
current task runner" flag into the constructor of AudioThread?

[alokp, 2017-04-28 17:31:13]

https://codereview.chromium.org/2784433002/diff/390001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/content/browser/browse...
content/browser/browser_main_loop.cc:1771: audio_system_.reset();
On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> Audio thread can have AudioSystem operations posted to it, and now they refer
to
> an invalid pointer.
> 
> Should we fix it according to the decision in the mail thread?
> 
> 1. AudioManager::Shutdown(): This shuts down the AudioManager and stops the
> audio thread.
> 2. delete AudioSystem
> 3. delete AudioThread
> 4. delete AudioManager

Good catch. IMO we should just shutdown the AM here and let ~BML destroy
AudioSystem and AudioManager in correct order. WDYT?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:348: audio_thread_->Stop();
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> Have you verified that hang monitor (l.308) will work correctly after this
> change?

Hang monitor runs on the file thread, which gets stopped before AM is shutdown,
so it should be OK.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:349: audio_thread_.reset();
On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> I believe we should only Stop() here and use another way to DCHEK if
> |audio_thread_| is stopped.
> 
> See my comment to BrowserMainLoop::DestroyAudioManager()

Even if we do not reset the |audio_thread| here, AM::GetTaskRunner would still
return null because base::Thread::task_runner() returns null after it has been
stopped.

I am fine with not deleting the audio_thread here, but am not sure if it buys us
anything.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:42: virtual ~AudioManager();
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> Could you specify that Shutdown() is mandatory before destruction?

I do have that comment near Shutdown. Do you think it should be repeated here? I
also have DCHECK ~AudioManager to verify that it was shutdown.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:191: return audio_thread_->GetTaskRunner();
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> Basing on AudioManager::Shutdown() |audio_thread_| can be nullptr.
> Basing on AudioThreadImpl::Stop() it can return nullptr now.
> 
> DCHECK(audio_thread_->GetTaskRunner())?
> And how do we make sure it's never called after Shutdown()?
> Should we re-think Shutdown() so that AudioManger remains in a valid state
after
> it?

This is a conscious design to return nullptr after AM has been Shutdown. This
prevents any tasks to be posted. Do you have any specific usecase that will
break by returning null from here?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:199: return audio_thread_->GetWorkerTaskRunner();
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> Same here

ditto :)

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
File media/audio/audio_system_impl_unittest.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:57:
base::MakeUnique<TestAudioThread>());
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> Can we have a bool parameter for AudioThreadImpl constructor instead?

I think this is cleaner. Did you have any specific concerns about
TestAudioThread being a separate class?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:79: ~AudioSystemImplTest() override {
audio_manager_->Shutdown(); }
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> Note that in this test the destruction sequence is different from the one in
the
> mail loop: Shutdown, AudioSystem deletion, AudioManager deletion.

This one is correct :) We need to fix BML as noted in the comment over there.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
File media/audio/audio_thread_impl.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
media/audio/audio_thread_impl.cc:31: task_runner_ = nullptr;
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> This does not guarantee that task_runner is deleted, right - since it's
> ref-counted, and some clients keep a ref ptr to it.

Clients keeping a ref-ptr is OK (although they do not need to and probably
should not). All tasks posted to task runner after the thread has stopped is
immediately discarded.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
media/audio/audio_thread_impl.cc:31: task_runner_ = nullptr;
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> Could you add explanations of how Stop() works in Mac case? Should we DCHECK
> that on Mac Stop() is called on task_runner_?

Yes I will add more comments and also add a thread-checker.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/test_audio...
File media/audio/test_audio_thread.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/test_audio...
media/audio/test_audio_thread.cc:13: :
task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> The purpose of this class needs to be explained in the header. Otherwise it's
> too tempting to use it for all tests, but some cases do require a separate
audio
> thread to be tested properly.
> 
> Actually, I would prefer to avoid having this test class. Can we pass "use
> current task runner" flag into the constructor of AudioThread?

I will add more comments. IMO having this separate from the main implementation
is cleaner.

[o1ka, 2017-05-02 16:16:01]

https://codereview.chromium.org/2784433002/diff/390001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/content/browser/browse...
content/browser/browser_main_loop.cc:1771: audio_system_.reset();
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > Audio thread can have AudioSystem operations posted to it, and now they
refer
> to
> > an invalid pointer.
> > 
> > Should we fix it according to the decision in the mail thread?
> > 
> > 1. AudioManager::Shutdown(): This shuts down the AudioManager and stops the
> > audio thread.
> > 2. delete AudioSystem
> > 3. delete AudioThread
> > 4. delete AudioManager
> 
> Good catch. IMO we should just shutdown the AM here and let ~BML destroy
> AudioSystem and AudioManager in correct order. WDYT?

sgtm

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:348: audio_thread_->Stop();
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > Have you verified that hang monitor (l.308) will work correctly after this
> > change?
> 
> Hang monitor runs on the file thread, which gets stopped before AM is
shutdown,
> so it should be OK.

I would say hang monitor *currently in content* runs on the file thread:
StartHangMonitorIfNeeded() receives task runner as a parameter; media code has
no guarantee here, has it? Should we take care of |monitor_task_runner_| and
tasks posted to it?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:349: audio_thread_.reset();
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > I believe we should only Stop() here and use another way to DCHEK if
> > |audio_thread_| is stopped.
> > 
> > See my comment to BrowserMainLoop::DestroyAudioManager()
> 
> Even if we do not reset the |audio_thread| here, AM::GetTaskRunner would still
> return null because base::Thread::task_runner() returns null after it has been
> stopped.

Could you clarify?
> 
> I am fine with not deleting the audio_thread here, but am not sure if it buys
us
> anything.

It would buy us that AM remains valid until it's deleted, and clients in media/
(like AudioSystemImpl) which are not aware of their lifetime with respect to
AudioManager::Shutdown() do not need to check AM::GetTaskRunner() result for
null.
It simplifies the code quite a lot. Our guarantee becomes "after Shutdown() no
operations are run on AudioManager thread, whatever clients do" - instead of
"..and clients must not post to that thread"

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:42: virtual ~AudioManager();
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > Could you specify that Shutdown() is mandatory before destruction?
> 
> I do have that comment near Shutdown. Do you think it should be repeated here?
I
> also have DCHECK ~AudioManager to verify that it was shutdown.

Right. Maybe move Shutdown() up next to Create(), so it does not get lost?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:191: return audio_thread_->GetTaskRunner();
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > Basing on AudioManager::Shutdown() |audio_thread_| can be nullptr.
> > Basing on AudioThreadImpl::Stop() it can return nullptr now.
> > 
> > DCHECK(audio_thread_->GetTaskRunner())?
> > And how do we make sure it's never called after Shutdown()?
> > Should we re-think Shutdown() so that AudioManger remains in a valid state
> after
> > it?
> 
> This is a conscious design to return nullptr after AM has been Shutdown. This
> prevents any tasks to be posted. Do you have any specific usecase that will
> break by returning null from here?

See my comment to AM::Shutdown()

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
File media/audio/audio_system_impl_unittest.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:57:
base::MakeUnique<TestAudioThread>());
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > Can we have a bool parameter for AudioThreadImpl constructor instead?
> 
> I think this is cleaner. Did you have any specific concerns about
> TestAudioThread being a separate class?

The problem with this particular test is that |use_audio_thread_| on Mac will
result in the same logic for true and false. But the intention is to test calls
when they are issued from another thread (it happens when |use_audio_thread_| is
true) / from the same (audio) thread (when it's false).

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:79: ~AudioSystemImplTest() override {
audio_manager_->Shutdown(); }
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > Note that in this test the destruction sequence is different from the one in
> the
> > mail loop: Shutdown, AudioSystem deletion, AudioManager deletion.
> 
> This one is correct :) We need to fix BML as noted in the comment over there.

Right, that's what I thought :)

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
File media/audio/audio_thread_impl.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
media/audio/audio_thread_impl.cc:31: task_runner_ = nullptr;
On 2017/04/28 17:31:13, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > This does not guarantee that task_runner is deleted, right - since it's
> > ref-counted, and some clients keep a ref ptr to it.
> 
> Clients keeping a ref-ptr is OK (although they do not need to and probably
> should not). All tasks posted to task runner after the thread has stopped is
> immediately discarded.

Acknowledged.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
media/audio/audio_thread_impl.cc:31: task_runner_ = nullptr;
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > Could you add explanations of how Stop() works in Mac case? Should we DCHECK
> > that on Mac Stop() is called on task_runner_?
> 
> Yes I will add more comments and also add a thread-checker.

sgtm

https://codereview.chromium.org/2784433002/diff/390001/media/audio/test_audio...
File media/audio/test_audio_thread.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/test_audio...
media/audio/test_audio_thread.cc:13: :
task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
On 2017/04/28 17:31:13, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > The purpose of this class needs to be explained in the header. Otherwise
it's
> > too tempting to use it for all tests, but some cases do require a separate
> audio
> > thread to be tested properly.
> > 
> > Actually, I would prefer to avoid having this test class. Can we pass "use
> > current task runner" flag into the constructor of AudioThread?
> 
> I will add more comments. IMO having this separate from the main
implementation
> is cleaner.

See my comment in audio_system_impl_unittest.cc. AudioThreadImpl +
TestAudioThread do not cover everything we need. We want to be able to specify
threading for tests in a platform-independent way.

[alokp, 2017-05-02 17:11:28]

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:348: audio_thread_->Stop();
On 2017/05/02 16:16:00, o1ka wrote:
> On 2017/04/28 17:31:12, alokp wrote:
> > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > Have you verified that hang monitor (l.308) will work correctly after this
> > > change?
> > 
> > Hang monitor runs on the file thread, which gets stopped before AM is
> shutdown,
> > so it should be OK.
> 
> I would say hang monitor *currently in content* runs on the file thread:
> StartHangMonitorIfNeeded() receives task runner as a parameter; media code has
> no guarantee here, has it? Should we take care of |monitor_task_runner_| and
> tasks posted to it?

You are right - it runs on IO thread - I misread the code. What do you mean by
"take care of tasks posted to it"?

It is currently safe now, since IO thread is stopped before the AM is  shutdown.
But may be we should explicitly suspend it to be more explicit?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:349: audio_thread_.reset();
On 2017/05/02 16:16:00, o1ka wrote:
> On 2017/04/28 17:31:12, alokp wrote:
> > On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > > I believe we should only Stop() here and use another way to DCHEK if
> > > |audio_thread_| is stopped.
> > > 
> > > See my comment to BrowserMainLoop::DestroyAudioManager()
> > 
> > Even if we do not reset the |audio_thread| here, AM::GetTaskRunner would
still
> > return null because base::Thread::task_runner() returns null after it has
been
> > stopped.
> 
> Could you clarify?
> > 
> > I am fine with not deleting the audio_thread here, but am not sure if it
buys
> us
> > anything.
> 
> It would buy us that AM remains valid until it's deleted, and clients in
media/
> (like AudioSystemImpl) which are not aware of their lifetime with respect to
> AudioManager::Shutdown() do not need to check AM::GetTaskRunner() result for
> null.
> It simplifies the code quite a lot. Our guarantee becomes "after Shutdown() no
> operations are run on AudioManager thread, whatever clients do" - instead of
> "..and clients must not post to that thread"
> 
> 

base::Thread::task_runner() returns NULL after it has been stopped. So I was
following the same model. Do we know if there are clients that call
AM::GetTaskRunner() after it has been shutdown? May be revisit it if there are
such clients? WDYT?

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:42: virtual ~AudioManager();
On 2017/05/02 16:16:00, o1ka wrote:
> On 2017/04/28 17:31:12, alokp wrote:
> > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > Could you specify that Shutdown() is mandatory before destruction?
> > 
> > I do have that comment near Shutdown. Do you think it should be repeated
here?
> I
> > also have DCHECK ~AudioManager to verify that it was shutdown.
> 
> Right. Maybe move Shutdown() up next to Create(), so it does not get lost?

Good Idea.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
File media/audio/audio_system_impl_unittest.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:57:
base::MakeUnique<TestAudioThread>());
On 2017/05/02 16:16:00, o1ka wrote:
> On 2017/04/28 17:31:12, alokp wrote:
> > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > Can we have a bool parameter for AudioThreadImpl constructor instead?
> > 
> > I think this is cleaner. Did you have any specific concerns about
> > TestAudioThread being a separate class?
> 
> The problem with this particular test is that |use_audio_thread_| on Mac will
> result in the same logic for true and false. But the intention is to test
calls
> when they are issued from another thread (it happens when |use_audio_thread_|
is
> true) / from the same (audio) thread (when it's false).

You are right, but what is the use for testing a codepath that is not exercised
in production? Also, there is a reason we use the main thread on mac. It is
because Core Audio needs to be used on the main thread. Isn't there a risk of
making the tests flaky if Core Audio runs on the audio thread?

[o1ka, 2017-05-02 21:42:24]

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:348: audio_thread_->Stop();
On 2017/05/02 17:11:28, alokp wrote:
> On 2017/05/02 16:16:00, o1ka wrote:
> > On 2017/04/28 17:31:12, alokp wrote:
> > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > Have you verified that hang monitor (l.308) will work correctly after
this
> > > > change?
> > > 
> > > Hang monitor runs on the file thread, which gets stopped before AM is
> > shutdown,
> > > so it should be OK.
> > 
> > I would say hang monitor *currently in content* runs on the file thread:
> > StartHangMonitorIfNeeded() receives task runner as a parameter; media code
has
> > no guarantee here, has it? Should we take care of |monitor_task_runner_| and
> > tasks posted to it?
> 
> You are right - it runs on IO thread - I misread the code. What do you mean by
> "take care of tasks posted to it"?
> 
> It is currently safe now, since IO thread is stopped before the AM is 
shutdown.
> But may be we should explicitly suspend it to be more explicit?

"take care of tasks posted to it" - I mean this
https://cs.chromium.org/chromium/src/media/audio/audio_manager.cc?dr=CSs&l=78.
We may want to cancel all the tasks holding a pointer to AM.

Yes - I believe we should suspend it.

I strongly believe that media/ should be as much self-sufficient as possible and
do not rely implicitly on lifetime of threads/objects hosted by content/.
Because all the implicit assumptions we make about that lead to complex code
which is hard to read and review. And if something goes wrong we end up with
hard-to-debug crashes.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:349: audio_thread_.reset();
On 2017/05/02 17:11:28, alokp wrote:
> On 2017/05/02 16:16:00, o1ka wrote:
> > On 2017/04/28 17:31:12, alokp wrote:
> > > On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > > > I believe we should only Stop() here and use another way to DCHEK if
> > > > |audio_thread_| is stopped.
> > > > 
> > > > See my comment to BrowserMainLoop::DestroyAudioManager()
> > > 
> > > Even if we do not reset the |audio_thread| here, AM::GetTaskRunner would
> still
> > > return null because base::Thread::task_runner() returns null after it has
> been
> > > stopped.
> > 
> > Could you clarify?
> > > 
> > > I am fine with not deleting the audio_thread here, but am not sure if it
> buys
> > us
> > > anything.
> > 
> > It would buy us that AM remains valid until it's deleted, and clients in
> media/
> > (like AudioSystemImpl) which are not aware of their lifetime with respect to
> > AudioManager::Shutdown() do not need to check AM::GetTaskRunner() result for
> > null.
> > It simplifies the code quite a lot. Our guarantee becomes "after Shutdown()
no
> > operations are run on AudioManager thread, whatever clients do" - instead of
> > "..and clients must not post to that thread"
> > 
> > 
> 
> base::Thread::task_runner() returns NULL after it has been stopped. So I was
> following the same model. Do we know if there are clients that call
> AM::GetTaskRunner() after it has been shutdown? May be revisit it if there are
> such clients? WDYT?

>Do we know if there are clients..
I'm not sure. To answer this question I need to dig into BrowserMainLoop
shutdown sequence and make sure no tasks using AM can be posted to the main
thread before AM::Shutdown and get executed before AM deletion. And that's
exactly what I want to avoid: I want us to stop worrying about the correct usage
of AM and make AM to take care about it.

While it's nice to follow base::Thread contract, I don't see why should we if it
complicates things: we are not inheriting from it, right? And AM does not have
to follow it as well.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
File media/audio/audio_system_impl_unittest.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:57:
base::MakeUnique<TestAudioThread>());
On 2017/05/02 17:11:28, alokp wrote:
> On 2017/05/02 16:16:00, o1ka wrote:
> > On 2017/04/28 17:31:12, alokp wrote:
> > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > Can we have a bool parameter for AudioThreadImpl constructor instead?
> > > 
> > > I think this is cleaner. Did you have any specific concerns about
> > > TestAudioThread being a separate class?
> > 
> > The problem with this particular test is that |use_audio_thread_| on Mac
will
> > result in the same logic for true and false. But the intention is to test
> calls
> > when they are issued from another thread (it happens when
|use_audio_thread_|
> is
> > true) / from the same (audio) thread (when it's false).
> 
> You are right, but what is the use for testing a codepath that is not
exercised
> in production? Also, there is a reason we use the main thread on mac. It is
> because Core Audio needs to be used on the main thread. Isn't there a risk of
> making the tests flaky if Core Audio runs on the audio thread?

AudioSystem can be accessed from any thread. In practice, it's usually accessed
from IO, sometimes from UI and potentially from AM thread - and on Mac AM thread
*is* UI thread. So on each platform we need to test AudioSystem access on AM
thread and on a non-AM thread.

It's a good point about CoreAudio not running on main thread on Mac here. I have
not seen any flakes so far though. We may want to change the tests to run AM on
the main thread and the rest of the logic on another thread. It will complicate
things a bit, so I would prefer to do it when we really see problems.

dalecurtis@ - WDYT?

[DaleCurtis, 2017-05-02 21:52:57]

On 2017/05/02 at 21:42:24, olka wrote:
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
> File media/audio/audio_manager.cc (right):
> 
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
> media/audio/audio_manager.cc:348: audio_thread_->Stop();
> On 2017/05/02 17:11:28, alokp wrote:
> > On 2017/05/02 16:16:00, o1ka wrote:
> > > On 2017/04/28 17:31:12, alokp wrote:
> > > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > > Have you verified that hang monitor (l.308) will work correctly after
this
> > > > > change?
> > > > 
> > > > Hang monitor runs on the file thread, which gets stopped before AM is
> > > shutdown,
> > > > so it should be OK.
> > > 
> > > I would say hang monitor *currently in content* runs on the file thread:
> > > StartHangMonitorIfNeeded() receives task runner as a parameter; media code
has
> > > no guarantee here, has it? Should we take care of |monitor_task_runner_|
and
> > > tasks posted to it?
> > 
> > You are right - it runs on IO thread - I misread the code. What do you mean
by
> > "take care of tasks posted to it"?
> > 
> > It is currently safe now, since IO thread is stopped before the AM is 
shutdown.
> > But may be we should explicitly suspend it to be more explicit?
> 
> "take care of tasks posted to it" - I mean this
https://cs.chromium.org/chromium/src/media/audio/audio_manager.cc?dr=CSs&l=78.
We may want to cancel all the tasks holding a pointer to AM.
> 
> Yes - I believe we should suspend it.
> 
> I strongly believe that media/ should be as much self-sufficient as possible
and do not rely implicitly on lifetime of threads/objects hosted by content/.
Because all the implicit assumptions we make about that lead to complex code
which is hard to read and review. And if something goes wrong we end up with
hard-to-debug crashes.
> 
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
> media/audio/audio_manager.cc:349: audio_thread_.reset();
> On 2017/05/02 17:11:28, alokp wrote:
> > On 2017/05/02 16:16:00, o1ka wrote:
> > > On 2017/04/28 17:31:12, alokp wrote:
> > > > On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > > > > I believe we should only Stop() here and use another way to DCHEK if
> > > > > |audio_thread_| is stopped.
> > > > > 
> > > > > See my comment to BrowserMainLoop::DestroyAudioManager()
> > > > 
> > > > Even if we do not reset the |audio_thread| here, AM::GetTaskRunner would
> > still
> > > > return null because base::Thread::task_runner() returns null after it
has
> > been
> > > > stopped.
> > > 
> > > Could you clarify?
> > > > 
> > > > I am fine with not deleting the audio_thread here, but am not sure if it
> > buys
> > > us
> > > > anything.
> > > 
> > > It would buy us that AM remains valid until it's deleted, and clients in
> > media/
> > > (like AudioSystemImpl) which are not aware of their lifetime with respect
to
> > > AudioManager::Shutdown() do not need to check AM::GetTaskRunner() result
for
> > > null.
> > > It simplifies the code quite a lot. Our guarantee becomes "after
Shutdown() no
> > > operations are run on AudioManager thread, whatever clients do" - instead
of
> > > "..and clients must not post to that thread"
> > > 
> > > 
> > 
> > base::Thread::task_runner() returns NULL after it has been stopped. So I was
> > following the same model. Do we know if there are clients that call
> > AM::GetTaskRunner() after it has been shutdown? May be revisit it if there
are
> > such clients? WDYT?
> 
> >Do we know if there are clients..
> I'm not sure. To answer this question I need to dig into BrowserMainLoop
shutdown sequence and make sure no tasks using AM can be posted to the main
thread before AM::Shutdown and get executed before AM deletion. And that's
exactly what I want to avoid: I want us to stop worrying about the correct usage
of AM and make AM to take care about it.
> 
> While it's nice to follow base::Thread contract, I don't see why should we if
it complicates things: we are not inheriting from it, right? And AM does not
have to follow it as well.
> 
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
> File media/audio/audio_system_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
> media/audio/audio_system_impl_unittest.cc:57:
base::MakeUnique<TestAudioThread>());
> On 2017/05/02 17:11:28, alokp wrote:
> > On 2017/05/02 16:16:00, o1ka wrote:
> > > On 2017/04/28 17:31:12, alokp wrote:
> > > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > > Can we have a bool parameter for AudioThreadImpl constructor instead?
> > > > 
> > > > I think this is cleaner. Did you have any specific concerns about
> > > > TestAudioThread being a separate class?
> > > 
> > > The problem with this particular test is that |use_audio_thread_| on Mac
will
> > > result in the same logic for true and false. But the intention is to test
> > calls
> > > when they are issued from another thread (it happens when
|use_audio_thread_|
> > is
> > > true) / from the same (audio) thread (when it's false).
> > 
> > You are right, but what is the use for testing a codepath that is not
exercised
> > in production? Also, there is a reason we use the main thread on mac. It is
> > because Core Audio needs to be used on the main thread. Isn't there a risk
of
> > making the tests flaky if Core Audio runs on the audio thread?
> 
> AudioSystem can be accessed from any thread. In practice, it's usually
accessed from IO, sometimes from UI and potentially from AM thread - and on Mac
AM thread *is* UI thread. So on each platform we need to test AudioSystem access
on AM thread and on a non-AM thread.
> 
> It's a good point about CoreAudio not running on main thread on Mac here. I
have not seen any flakes so far though. We may want to change the tests to run
AM on the main thread and the rest of the logic on another thread. It will
complicate things a bit, so I would prefer to do it when we really see problems.
> 
> dalecurtis@ - WDYT?

In practice we don't have a lot of tests which actually touch CoreAudio, so it
hasn't been an issue AFAIK. That said, I support making the tests for macOS do
the right thing if it's not difficult -- otherwise I wouldn't worry about it.

[o1ka, 2017-05-02 22:05:39]

On 2017/05/02 21:52:57, DaleCurtis wrote:
> On 2017/05/02 at 21:42:24, olka wrote:
> >
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
> > File media/audio/audio_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
> > media/audio/audio_manager.cc:348: audio_thread_->Stop();
> > On 2017/05/02 17:11:28, alokp wrote:
> > > On 2017/05/02 16:16:00, o1ka wrote:
> > > > On 2017/04/28 17:31:12, alokp wrote:
> > > > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > > > Have you verified that hang monitor (l.308) will work correctly
after
> this
> > > > > > change?
> > > > > 
> > > > > Hang monitor runs on the file thread, which gets stopped before AM is
> > > > shutdown,
> > > > > so it should be OK.
> > > > 
> > > > I would say hang monitor *currently in content* runs on the file thread:
> > > > StartHangMonitorIfNeeded() receives task runner as a parameter; media
code
> has
> > > > no guarantee here, has it? Should we take care of |monitor_task_runner_|
> and
> > > > tasks posted to it?
> > > 
> > > You are right - it runs on IO thread - I misread the code. What do you
mean
> by
> > > "take care of tasks posted to it"?
> > > 
> > > It is currently safe now, since IO thread is stopped before the AM is 
> shutdown.
> > > But may be we should explicitly suspend it to be more explicit?
> > 
> > "take care of tasks posted to it" - I mean this
> https://cs.chromium.org/chromium/src/media/audio/audio_manager.cc?dr=CSs&l=78.
> We may want to cancel all the tasks holding a pointer to AM.
> > 
> > Yes - I believe we should suspend it.
> > 
> > I strongly believe that media/ should be as much self-sufficient as possible
> and do not rely implicitly on lifetime of threads/objects hosted by content/.
> Because all the implicit assumptions we make about that lead to complex code
> which is hard to read and review. And if something goes wrong we end up with
> hard-to-debug crashes.
> > 
> >
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
> > media/audio/audio_manager.cc:349: audio_thread_.reset();
> > On 2017/05/02 17:11:28, alokp wrote:
> > > On 2017/05/02 16:16:00, o1ka wrote:
> > > > On 2017/04/28 17:31:12, alokp wrote:
> > > > > On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > > > > > I believe we should only Stop() here and use another way to DCHEK if
> > > > > > |audio_thread_| is stopped.
> > > > > > 
> > > > > > See my comment to BrowserMainLoop::DestroyAudioManager()
> > > > > 
> > > > > Even if we do not reset the |audio_thread| here, AM::GetTaskRunner
would
> > > still
> > > > > return null because base::Thread::task_runner() returns null after it
> has
> > > been
> > > > > stopped.
> > > > 
> > > > Could you clarify?
> > > > > 
> > > > > I am fine with not deleting the audio_thread here, but am not sure if
it
> > > buys
> > > > us
> > > > > anything.
> > > > 
> > > > It would buy us that AM remains valid until it's deleted, and clients in
> > > media/
> > > > (like AudioSystemImpl) which are not aware of their lifetime with
respect
> to
> > > > AudioManager::Shutdown() do not need to check AM::GetTaskRunner() result
> for
> > > > null.
> > > > It simplifies the code quite a lot. Our guarantee becomes "after
> Shutdown() no
> > > > operations are run on AudioManager thread, whatever clients do" -
instead
> of
> > > > "..and clients must not post to that thread"
> > > > 
> > > > 
> > > 
> > > base::Thread::task_runner() returns NULL after it has been stopped. So I
was
> > > following the same model. Do we know if there are clients that call
> > > AM::GetTaskRunner() after it has been shutdown? May be revisit it if there
> are
> > > such clients? WDYT?
> > 
> > >Do we know if there are clients..
> > I'm not sure. To answer this question I need to dig into BrowserMainLoop
> shutdown sequence and make sure no tasks using AM can be posted to the main
> thread before AM::Shutdown and get executed before AM deletion. And that's
> exactly what I want to avoid: I want us to stop worrying about the correct
usage
> of AM and make AM to take care about it.
> > 
> > While it's nice to follow base::Thread contract, I don't see why should we
if
> it complicates things: we are not inheriting from it, right? And AM does not
> have to follow it as well.
> > 
> >
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
> > File media/audio/audio_system_impl_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
> > media/audio/audio_system_impl_unittest.cc:57:
> base::MakeUnique<TestAudioThread>());
> > On 2017/05/02 17:11:28, alokp wrote:
> > > On 2017/05/02 16:16:00, o1ka wrote:
> > > > On 2017/04/28 17:31:12, alokp wrote:
> > > > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > > > Can we have a bool parameter for AudioThreadImpl constructor
instead?
> > > > > 
> > > > > I think this is cleaner. Did you have any specific concerns about
> > > > > TestAudioThread being a separate class?
> > > > 
> > > > The problem with this particular test is that |use_audio_thread_| on Mac
> will
> > > > result in the same logic for true and false. But the intention is to
test
> > > calls
> > > > when they are issued from another thread (it happens when
> |use_audio_thread_|
> > > is
> > > > true) / from the same (audio) thread (when it's false).
> > > 
> > > You are right, but what is the use for testing a codepath that is not
> exercised
> > > in production? Also, there is a reason we use the main thread on mac. It
is
> > > because Core Audio needs to be used on the main thread. Isn't there a risk
> of
> > > making the tests flaky if Core Audio runs on the audio thread?
> > 
> > AudioSystem can be accessed from any thread. In practice, it's usually
> accessed from IO, sometimes from UI and potentially from AM thread - and on
Mac
> AM thread *is* UI thread. So on each platform we need to test AudioSystem
access
> on AM thread and on a non-AM thread.
> > 
> > It's a good point about CoreAudio not running on main thread on Mac here. I
> have not seen any flakes so far though. We may want to change the tests to run
> AM on the main thread and the rest of the logic on another thread. It will
> complicate things a bit, so I would prefer to do it when we really see
problems.
> > 
> > dalecurtis@ - WDYT?
> 
> In practice we don't have a lot of tests which actually touch CoreAudio, so it
> hasn't been an issue AFAIK. That said, I support making the tests for macOS do
> the right thing if it's not difficult -- otherwise I wouldn't worry about it.

Oh, right, this particular test does not run CoreAudio either: it uses a mock
AM.
So non-main thread should not be an issue here. But we should probably add a
comment explaining that.

[alokp, 2017-05-09 18:10:12]

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

[commit-bot: I haz the power, 2017-05-09 18:11:25]

Dry run: CQ is trying da patch.

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

[alokp, 2017-05-09 19:06:15]

https://codereview.chromium.org/2784433002/diff/390001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/content/browser/browse...
content/browser/browser_main_loop.cc:1771: audio_system_.reset();
On 2017/05/02 16:16:00, o1ka wrote:
> On 2017/04/28 17:31:12, alokp wrote:
> > On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > > Audio thread can have AudioSystem operations posted to it, and now they
> refer
> > to
> > > an invalid pointer.
> > > 
> > > Should we fix it according to the decision in the mail thread?
> > > 
> > > 1. AudioManager::Shutdown(): This shuts down the AudioManager and stops
the
> > > audio thread.
> > > 2. delete AudioSystem
> > > 3. delete AudioThread
> > > 4. delete AudioManager
> > 
> > Good catch. IMO we should just shutdown the AM here and let ~BML destroy
> > AudioSystem and AudioManager in correct order. WDYT?
> 
> sgtm

Done.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:348: audio_thread_->Stop();
On 2017/05/02 21:42:23, o1ka wrote:
> On 2017/05/02 17:11:28, alokp wrote:
> > On 2017/05/02 16:16:00, o1ka wrote:
> > > On 2017/04/28 17:31:12, alokp wrote:
> > > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > > Have you verified that hang monitor (l.308) will work correctly after
> this
> > > > > change?
> > > > 
> > > > Hang monitor runs on the file thread, which gets stopped before AM is
> > > shutdown,
> > > > so it should be OK.
> > > 
> > > I would say hang monitor *currently in content* runs on the file thread:
> > > StartHangMonitorIfNeeded() receives task runner as a parameter; media code
> has
> > > no guarantee here, has it? Should we take care of |monitor_task_runner_|
and
> > > tasks posted to it?
> > 
> > You are right - it runs on IO thread - I misread the code. What do you mean
by
> > "take care of tasks posted to it"?
> > 
> > It is currently safe now, since IO thread is stopped before the AM is 
> shutdown.
> > But may be we should explicitly suspend it to be more explicit?
> 
> "take care of tasks posted to it" - I mean this
> https://cs.chromium.org/chromium/src/media/audio/audio_manager.cc?dr=CSs&l=78.
> We may want to cancel all the tasks holding a pointer to AM.
> 
> Yes - I believe we should suspend it.
> 
> I strongly believe that media/ should be as much self-sufficient as possible
and
> do not rely implicitly on lifetime of threads/objects hosted by content/.
> Because all the implicit assumptions we make about that lead to complex code
> which is hard to read and review. And if something goes wrong we end up with
> hard-to-debug crashes.
> 

I looked at this more closely. It would be easier to suspend hang monitoring if
we moved the ownership of hang monitor to audio manager. I will do this in a
separate patch though. For now I have added a TODO.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:349: audio_thread_.reset();
On 2017/05/02 21:42:23, o1ka wrote:
> On 2017/05/02 17:11:28, alokp wrote:
> > On 2017/05/02 16:16:00, o1ka wrote:
> > > On 2017/04/28 17:31:12, alokp wrote:
> > > > On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > > > > I believe we should only Stop() here and use another way to DCHEK if
> > > > > |audio_thread_| is stopped.
> > > > > 
> > > > > See my comment to BrowserMainLoop::DestroyAudioManager()
> > > > 
> > > > Even if we do not reset the |audio_thread| here, AM::GetTaskRunner would
> > still
> > > > return null because base::Thread::task_runner() returns null after it
has
> > been
> > > > stopped.
> > > 
> > > Could you clarify?
> > > > 
> > > > I am fine with not deleting the audio_thread here, but am not sure if it
> > buys
> > > us
> > > > anything.
> > > 
> > > It would buy us that AM remains valid until it's deleted, and clients in
> > media/
> > > (like AudioSystemImpl) which are not aware of their lifetime with respect
to
> > > AudioManager::Shutdown() do not need to check AM::GetTaskRunner() result
for
> > > null.
> > > It simplifies the code quite a lot. Our guarantee becomes "after
Shutdown()
> no
> > > operations are run on AudioManager thread, whatever clients do" - instead
of
> > > "..and clients must not post to that thread"
> > > 
> > > 
> > 
> > base::Thread::task_runner() returns NULL after it has been stopped. So I was
> > following the same model. Do we know if there are clients that call
> > AM::GetTaskRunner() after it has been shutdown? May be revisit it if there
are
> > such clients? WDYT?
> 
> >Do we know if there are clients..
> I'm not sure. To answer this question I need to dig into BrowserMainLoop
> shutdown sequence and make sure no tasks using AM can be posted to the main
> thread before AM::Shutdown and get executed before AM deletion. And that's
> exactly what I want to avoid: I want us to stop worrying about the correct
usage
> of AM and make AM to take care about it.
> 
> While it's nice to follow base::Thread contract, I don't see why should we if
it
> complicates things: we are not inheriting from it, right? And AM does not have
> to follow it as well.

Done.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:191: return audio_thread_->GetTaskRunner();
On 2017/05/02 16:16:00, o1ka wrote:
> On 2017/04/28 17:31:12, alokp wrote:
> > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > Basing on AudioManager::Shutdown() |audio_thread_| can be nullptr.
> > > Basing on AudioThreadImpl::Stop() it can return nullptr now.
> > > 
> > > DCHECK(audio_thread_->GetTaskRunner())?
> > > And how do we make sure it's never called after Shutdown()?
> > > Should we re-think Shutdown() so that AudioManger remains in a valid state
> > after
> > > it?
> > 
> > This is a conscious design to return nullptr after AM has been Shutdown.
This
> > prevents any tasks to be posted. Do you have any specific usecase that will
> > break by returning null from here?
> 
> See my comment to AM::Shutdown()

Now I do not reset the thread or task runner on shutdown.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.h:199: return audio_thread_->GetWorkerTaskRunner();
On 2017/04/28 17:31:12, alokp wrote:
> On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > Same here
> 
> ditto :)

Done.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
File media/audio/audio_system_impl_unittest.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_syst...
media/audio/audio_system_impl_unittest.cc:57:
base::MakeUnique<TestAudioThread>());
On 2017/05/02 21:42:23, o1ka wrote:
> On 2017/05/02 17:11:28, alokp wrote:
> > On 2017/05/02 16:16:00, o1ka wrote:
> > > On 2017/04/28 17:31:12, alokp wrote:
> > > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > > Can we have a bool parameter for AudioThreadImpl constructor instead?
> > > > 
> > > > I think this is cleaner. Did you have any specific concerns about
> > > > TestAudioThread being a separate class?
> > > 
> > > The problem with this particular test is that |use_audio_thread_| on Mac
> will
> > > result in the same logic for true and false. But the intention is to test
> > calls
> > > when they are issued from another thread (it happens when
> |use_audio_thread_|
> > is
> > > true) / from the same (audio) thread (when it's false).
> > 
> > You are right, but what is the use for testing a codepath that is not
> exercised
> > in production? Also, there is a reason we use the main thread on mac. It is
> > because Core Audio needs to be used on the main thread. Isn't there a risk
of
> > making the tests flaky if Core Audio runs on the audio thread?
> 
> AudioSystem can be accessed from any thread. In practice, it's usually
accessed
> from IO, sometimes from UI and potentially from AM thread - and on Mac AM
thread
> *is* UI thread. So on each platform we need to test AudioSystem access on AM
> thread and on a non-AM thread.
> 
> It's a good point about CoreAudio not running on main thread on Mac here. I
have
> not seen any flakes so far though. We may want to change the tests to run AM
on
> the main thread and the rest of the logic on another thread. It will
complicate
> things a bit, so I would prefer to do it when we really see problems.
> 
> dalecurtis@ - WDYT?

I added a bool to TestAudioThread constructor and used it here.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
File media/audio/audio_thread.h (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_thre...
media/audio/audio_thread.h:23: // Both - GetTaskRunner and GetWorkerTaskRunner -
will return null after this
On 2017/04/28 13:24:20, o1ka wrote:
> Should AudioManager check for null?

Removed comment. Now we continue to return a task runner after stop.

https://codereview.chromium.org/2784433002/diff/390001/media/audio/test_audio...
File media/audio/test_audio_thread.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/test_audio...
media/audio/test_audio_thread.cc:13: :
task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
On 2017/05/02 16:16:01, o1ka wrote:
> On 2017/04/28 17:31:13, alokp wrote:
> > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > The purpose of this class needs to be explained in the header. Otherwise
> it's
> > > too tempting to use it for all tests, but some cases do require a separate
> > audio
> > > thread to be tested properly.
> > > 
> > > Actually, I would prefer to avoid having this test class. Can we pass "use
> > > current task runner" flag into the constructor of AudioThread?
> > 
> > I will add more comments. IMO having this separate from the main
> implementation
> > is cleaner.
> 
> See my comment in audio_system_impl_unittest.cc. AudioThreadImpl +
> TestAudioThread do not cover everything we need. We want to be able to specify
> threading for tests in a platform-independent way.

Done.

[o1ka, 2017-05-10 15:57:57]

lgtm % comments below.
Thanks for doing this - it helps us a lot!

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:348: audio_thread_->Stop();
On 2017/05/09 19:06:15, alokp wrote:
> On 2017/05/02 21:42:23, o1ka wrote:
> > On 2017/05/02 17:11:28, alokp wrote:
> > > On 2017/05/02 16:16:00, o1ka wrote:
> > > > On 2017/04/28 17:31:12, alokp wrote:
> > > > > On 2017/04/28 13:24:20, o1ka OOO till May 2nd wrote:
> > > > > > Have you verified that hang monitor (l.308) will work correctly
after
> > this
> > > > > > change?
> > > > > 
> > > > > Hang monitor runs on the file thread, which gets stopped before AM is
> > > > shutdown,
> > > > > so it should be OK.
> > > > 
> > > > I would say hang monitor *currently in content* runs on the file thread:
> > > > StartHangMonitorIfNeeded() receives task runner as a parameter; media
code
> > has
> > > > no guarantee here, has it? Should we take care of |monitor_task_runner_|
> and
> > > > tasks posted to it?
> > > 
> > > You are right - it runs on IO thread - I misread the code. What do you
mean
> by
> > > "take care of tasks posted to it"?
> > > 
> > > It is currently safe now, since IO thread is stopped before the AM is 
> > shutdown.
> > > But may be we should explicitly suspend it to be more explicit?
> > 
> > "take care of tasks posted to it" - I mean this
> >
https://cs.chromium.org/chromium/src/media/audio/audio_manager.cc?dr=CSs&l=78.
> > We may want to cancel all the tasks holding a pointer to AM.
> > 
> > Yes - I believe we should suspend it.
> > 
> > I strongly believe that media/ should be as much self-sufficient as possible
> and
> > do not rely implicitly on lifetime of threads/objects hosted by content/.
> > Because all the implicit assumptions we make about that lead to complex code
> > which is hard to read and review. And if something goes wrong we end up with
> > hard-to-debug crashes.
> > 
> 
> I looked at this more closely. It would be easier to suspend hang monitoring
if
> we moved the ownership of hang monitor to audio manager. I will do this in a
> separate patch though. For now I have added a TODO.

sgtm

https://codereview.chromium.org/2784433002/diff/390001/media/audio/audio_mana...
media/audio/audio_manager.cc:349: audio_thread_.reset();
On 2017/05/09 19:06:15, alokp wrote:
> On 2017/05/02 21:42:23, o1ka wrote:
> > On 2017/05/02 17:11:28, alokp wrote:
> > > On 2017/05/02 16:16:00, o1ka wrote:
> > > > On 2017/04/28 17:31:12, alokp wrote:
> > > > > On 2017/04/28 13:24:19, o1ka OOO till May 2nd wrote:
> > > > > > I believe we should only Stop() here and use another way to DCHEK if
> > > > > > |audio_thread_| is stopped.
> > > > > > 
> > > > > > See my comment to BrowserMainLoop::DestroyAudioManager()
> > > > > 
> > > > > Even if we do not reset the |audio_thread| here, AM::GetTaskRunner
would
> > > still
> > > > > return null because base::Thread::task_runner() returns null after it
> has
> > > been
> > > > > stopped.
> > > > 
> > > > Could you clarify?
> > > > > 
> > > > > I am fine with not deleting the audio_thread here, but am not sure if
it
> > > buys
> > > > us
> > > > > anything.
> > > > 
> > > > It would buy us that AM remains valid until it's deleted, and clients in
> > > media/
> > > > (like AudioSystemImpl) which are not aware of their lifetime with
respect
> to
> > > > AudioManager::Shutdown() do not need to check AM::GetTaskRunner() result
> for
> > > > null.
> > > > It simplifies the code quite a lot. Our guarantee becomes "after
> Shutdown()
> > no
> > > > operations are run on AudioManager thread, whatever clients do" -
instead
> of
> > > > "..and clients must not post to that thread"
> > > > 
> > > > 
> > > 
> > > base::Thread::task_runner() returns NULL after it has been stopped. So I
was
> > > following the same model. Do we know if there are clients that call
> > > AM::GetTaskRunner() after it has been shutdown? May be revisit it if there
> are
> > > such clients? WDYT?
> > 
> > >Do we know if there are clients..
> > I'm not sure. To answer this question I need to dig into BrowserMainLoop
> > shutdown sequence and make sure no tasks using AM can be posted to the main
> > thread before AM::Shutdown and get executed before AM deletion. And that's
> > exactly what I want to avoid: I want us to stop worrying about the correct
> usage
> > of AM and make AM to take care about it.
> > 
> > While it's nice to follow base::Thread contract, I don't see why should we
if
> it
> > complicates things: we are not inheriting from it, right? And AM does not
have
> > to follow it as well.
> 
> Done.

Thanks!

https://codereview.chromium.org/2784433002/diff/430001/media/audio/alsa/audio...
File media/audio/alsa/audio_manager_alsa.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/alsa/audio...
media/audio/alsa/audio_manager_alsa.cc:92: wrapper_.reset();
Add a comment why we must do it now?

https://codereview.chromium.org/2784433002/diff/430001/media/audio/android/au...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/android/au...
media/audio/android/audio_manager_android.cc:79: if
(!j_audio_manager_.is_null()) {
Add a comment why we must do it now on audio thread?

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_mana...
media/audio/audio_manager.h:279: bool shutdown_;  // True after |this| has been
shutdown.
= false?

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_mana...
File media/audio/audio_manager_base.cc (left):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:362: CHECK(input_streams_.empty());
Why is it removed?

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
File media/audio/audio_thread.h (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
media/audio/audio_thread.h:25: // Returns the task runner used for audio IO.
State that both methods should always return valid task runners (not null)?

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
media/audio/audio_thread.h:30: // UI loop with the audio IO loop.
Move audio_manager.h l.49 comment here instead? I think that one explains the
meaning of task runners better.

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
File media/audio/audio_thread_impl.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
media/audio/audio_thread_impl.cc:27: AudioThreadImpl::~AudioThreadImpl() {}
DCHECK that it's called on |task_runner_| on Mac?
And add a comment that on Mac we can still have tasks posted on on
|task_runner_|  after Stop() (since it's the main thread task runner and we
don't stop the main thread) - but it's fine since AudioThreadImpl destructor
will be called on the main thread during main message loop shutdown?

https://codereview.chromium.org/2784433002/diff/430001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:56: // The Pulse objects are the last
things to be destroyed since Shutdown()
Fix a comment?

https://codereview.chromium.org/2784433002/diff/430001/media/audio/win/audio_...
File media/audio/win/audio_manager_win.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/win/audio_...
media/audio/win/audio_manager_win.cc:160: output_device_listener_.reset();
Add a comment why it's done here?

[alokp, 2017-05-10 18:04:04]

https://codereview.chromium.org/2784433002/diff/430001/media/audio/alsa/audio...
File media/audio/alsa/audio_manager_alsa.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/alsa/audio...
media/audio/alsa/audio_manager_alsa.cc:92: wrapper_.reset();
On 2017/05/10 15:57:56, o1ka wrote:
> Add a comment why we must do it now?

Actually we do not need to explicitly destroy alsa wrapper on the audio thread.
Reverted changes in this file.

https://codereview.chromium.org/2784433002/diff/430001/media/audio/android/au...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/android/au...
media/audio/android/audio_manager_android.cc:79: if
(!j_audio_manager_.is_null()) {
On 2017/05/10 15:57:56, o1ka wrote:
> Add a comment why we must do it now on audio thread?

Done.

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_mana...
media/audio/audio_manager.h:279: bool shutdown_;  // True after |this| has been
shutdown.
On 2017/05/10 15:57:57, o1ka wrote:
> = false?

Done.

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_mana...
File media/audio/audio_manager_base.cc (left):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_mana...
media/audio/audio_manager_base.cc:362: CHECK(input_streams_.empty());
On 2017/05/10 15:57:57, o1ka wrote:
> Why is it removed?

I meant to remove it in another patch. It somehow slipped through. Reverted.

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
File media/audio/audio_thread.h (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
media/audio/audio_thread.h:25: // Returns the task runner used for audio IO.
On 2017/05/10 15:57:57, o1ka wrote:
> State that both methods should always return valid task runners (not null)?

Done.

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
media/audio/audio_thread.h:30: // UI loop with the audio IO loop.
On 2017/05/10 15:57:57, o1ka wrote:
> Move audio_manager.h l.49 comment here instead? I think that one explains the
> meaning of task runners better.

I do not see a difference in the comments. What exactly is missing here?

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
File media/audio/audio_thread_impl.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
media/audio/audio_thread_impl.cc:27: AudioThreadImpl::~AudioThreadImpl() {}
On 2017/05/10 15:57:57, o1ka wrote:
> DCHECK that it's called on |task_runner_| on Mac?
> And add a comment that on Mac we can still have tasks posted on on
> |task_runner_|  after Stop() (since it's the main thread task runner and we
> don't stop the main thread) - but it's fine since AudioThreadImpl destructor
> will be called on the main thread during main message loop shutdown?

I do not think we want to verify anything special for mac. This class must be
created and destroyed on the same thread on all platforms. I added thread
checker to validate that. This also validates what you wanted to validate on
mac.

Added comments about mac-specific issues in Stop.

https://codereview.chromium.org/2784433002/diff/430001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:56: // The Pulse objects are the last
things to be destroyed since Shutdown()
On 2017/05/10 15:57:57, o1ka wrote:
> Fix a comment?

Done.

https://codereview.chromium.org/2784433002/diff/430001/media/audio/win/audio_...
File media/audio/win/audio_manager_win.cc (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/win/audio_...
media/audio/win/audio_manager_win.cc:160: output_device_listener_.reset();
On 2017/05/10 15:57:57, o1ka wrote:
> Add a comment why it's done here?

Done.

[alokp, 2017-05-10 18:05:28]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-05-10 18:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, olka@chromium.org
Link to the patchset: https://codereview.chromium.org/2784433002/#ps470001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-10 18:07:32]

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-05-10 18:31:48]

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

[commit-bot: I haz the power, 2017-05-10 18:31:50]

Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[alokp, 2017-05-10 18:39:58]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-05-10 18:39:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, olka@chromium.org
Link to the patchset: https://codereview.chromium.org/2784433002/#ps490001
(title: "adds missing include")

[commit-bot: I haz the power, 2017-05-10 18:41:14]

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-05-10 18:57:53]

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

[commit-bot: I haz the power, 2017-05-10 18:57:54]

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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alokp, 2017-05-10 19:42:09]

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

[alokp, 2017-05-10 19:42:11]

+jam: content/

[jam, 2017-05-10 22:17:24]

content lgtm (didnt look at media subdir)

[alokp, 2017-05-10 22:53:09]

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

[commit-bot: I haz the power, 2017-05-10 22:54:15]

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-05-10 23:04:52]

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

[commit-bot: I haz the power, 2017-05-10 23:04:54]

Dry run: Try jobs failed on following builders:
  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-05-10 23:23:18]

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

[commit-bot: I haz the power, 2017-05-10 23:24: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-05-11 00:41:12]

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

[commit-bot: I haz the power, 2017-05-11 00:41:14]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[alokp, 2017-05-11 12:27:56]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-05-11 12:27:57]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dalecurtis@chromium.org, olka@chromium.org
Link to the patchset: https://codereview.chromium.org/2784433002/#ps530001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-11 12:28:21]

CQ is trying da patch.

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

[o1ka, 2017-05-11 13:44:12]

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
File media/audio/audio_thread.h (right):

https://codereview.chromium.org/2784433002/diff/430001/media/audio/audio_thre...
media/audio/audio_thread.h:30: // UI loop with the audio IO loop.
On 2017/05/10 18:04:03, alokp wrote:
> On 2017/05/10 15:57:57, o1ka wrote:
> > Move audio_manager.h l.49 comment here instead? I think that one explains
the
> > meaning of task runners better.
> 
> I do not see a difference in the comments. What exactly is missing here?

The other comment makes more sense, since there is no concept of UI and IO loops
in media.

[commit-bot: I haz the power, 2017-05-11 14:13:44]

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

[commit-bot: I haz the power, 2017-05-11 14:13:46]

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

[alokp, 2017-05-11 17:04:30]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2017-05-11 17:05:21]

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-05-11 19:19:01]

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

[commit-bot: I haz the power, 2017-05-11 19:19:04]

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

[alokp, 2017-05-11 22:13:48]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2017-05-11 22:15:15]

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-05-12 00:42:19]

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

[commit-bot: I haz the power, 2017-05-12 00:42:21]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alokp, 2017-05-12 17:46:49]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-05-12 17:46:50]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dalecurtis@chromium.org, olka@chromium.org
Link to the patchset: https://codereview.chromium.org/2784433002/#ps570001
(title: "fixes media_unittests on mac")

[commit-bot: I haz the power, 2017-05-12 17:48:22]

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-05-12 20:52:28]

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

[commit-bot: I haz the power, 2017-05-12 20:52:30]

Try jobs failed on following builders:
  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-05-12 23:39:21]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-05-12 23:39:22]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dalecurtis@chromium.org, olka@chromium.org
Link to the patchset: https://codereview.chromium.org/2784433002/#ps590001
(title: "fixes content_unittests on mac")

[commit-bot: I haz the power, 2017-05-12 23:40:03]

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-05-12 23:44:52]

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

[commit-bot: I haz the power, 2017-05-12 23:44:55]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[alokp, 2017-05-13 00:19:32]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-05-13 00:19:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dalecurtis@chromium.org, olka@chromium.org
Link to the patchset: https://codereview.chromium.org/2784433002/#ps610001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-13 00:20:01]

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

CQ is committing da patch.

Bot data: {"patchset_id": 610001, "attempt_start_ts": 1494634772168800,
"parent_rev": "ff06c2bf52f825866cb1c1c928bc6510da49ad9c", "commit_rev":
"2cbd4fcab300af5aa4ef76752673901d58da0f24"}

[commit-bot: I haz the power, 2017-05-13 04:13:41]

Description was changed from

==========
Ensures that audio tasks cannot run after AudioManager is deleted.

AudioManager is now deleted on the main thread instead of the audio
thread. It is Shutdown on the audio thread which allows it to release
resources on the correct thread. Audio thread is then stopped before
audio manager is deleted so that all tasks in the audio thread queue
runs and no more tasks can be posted.

BUG=705455
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

==========
Ensures that audio tasks cannot run after AudioManager is deleted.

AudioManager is now deleted on the main thread instead of the audio
thread. It is Shutdown on the audio thread which allows it to release
resources on the correct thread. Audio thread is then stopped before
audio manager is deleted so that all tasks in the audio thread queue
runs and no more tasks can be posted.

BUG=705455
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/2784433002
Cr-Commit-Position: refs/heads/master@{#471558}
Committed:
https://chromium.googlesource.com/chromium/src/+/2cbd4fcab300af5aa4ef76752673...
==========

[commit-bot: I haz the power, 2017-05-13 04:13:43]

Committed patchset #32 (id:610001) as
https://chromium.googlesource.com/chromium/src/+/2cbd4fcab300af5aa4ef76752673...