Ensures that audio tasks cannot run after AudioManager is deleted.

media/audio/audio_manager.h

Index: media/audio/audio_manager.h
diff --git a/media/audio/audio_manager.h b/media/audio/audio_manager.h
index cd91b399d6d4595ce60faf0131baa65f9cfd8bca..00a7b8abbb4690b0d66e77d6a8914a2bebf24e04 100644
--- a/media/audio/audio_manager.h
+++ b/media/audio/audio_manager.h
@@ -14,9 +14,11 @@
 #include "base/memory/ref_counted.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/strings/string16.h"
+#include "base/threading/thread_checker.h"
 #include "build/build_config.h"
 #include "media/audio/audio_device_description.h"
 #include "media/audio/audio_logging.h"
+#include "media/audio/audio_thread.h"
 #include "media/base/audio_parameters.h"
 
 namespace base {
@@ -30,13 +32,6 @@ class AudioInputStream;
 class AudioManager;
 class AudioOutputStream;
 
-class MEDIA_EXPORT AudioManagerDeleter {
- public:
-  void operator()(const AudioManager* instance) const;
-};
-using ScopedAudioManagerPtr =
-    std::unique_ptr<AudioManager, AudioManagerDeleter>;
-
 // Manages all audio resources.  Provides some convenience functions that avoid
 // the need to provide iterators over the existing streams.
 //
@@ -44,35 +39,33 @@ using ScopedAudioManagerPtr =
 // thread hang is detected, it is reported to UMA.
 class MEDIA_EXPORT AudioManager {
  public:
+  virtual ~AudioManager();

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

Could you specify that Shutdown() is mandatory before destruction?

[alokp, 2017/04/28 17:31:12]

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.

[o1ka, 2017/05/02 16:16:00]

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

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

Good Idea.

+
   // Construct the audio manager; only one instance is allowed.
-  // The returned instance must be deleted on AudioManager::GetTaskRunnner().
   //
   // The manager will forward CreateAudioLog() calls to the provided
   // AudioLogFactory; as such |audio_log_factory| must outlive the AudioManager.
   //
-  // The manager will use |task_runner| for audio IO. This same task runner
-  // is returned by GetTaskRunner().
-  // On OS_MACOSX, CoreAudio requires that |task_runner| must belong to the
-  // main thread of the process, which in our case is sadly the browser UI
-  // thread. Failure to execute calls on the right thread leads to crashes and
-  // odd behavior. See http://crbug.com/158170.
+  // The manager will use |audio_thread->GetTaskRunner()| for audio IO.
+  // On OS_MACOSX, CoreAudio requires that |audio_thread->GetTaskRunner()|
+  // must belong to the main thread of the process, which in our case is sadly
+  // the browser UI thread. Failure to execute calls on the right thread leads
+  // to crashes and odd behavior. See http://crbug.com/158170.
   //
-  // The manager will use |worker_task_runner| for heavyweight tasks.
-  // The |worker_task_runner| may be the same as |task_runner|. This same
-  // task runner is returned by GetWorkerTaskRunner.
+  // The manager will use |audio_thread->GetWorkerTaskRunner()| for heavyweight
+  // tasks. The |audio_thread->GetWorkerTaskRunner()| may be the same as
+  // |audio_thread->GetTaskRunner()|.
   //
   // |file_task_runner| is used for audio debug recordings and is the task
   // runner to do file output operations on.
-  static ScopedAudioManagerPtr Create(
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-      scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner,
+  static std::unique_ptr<AudioManager> Create(
+      std::unique_ptr<AudioThread> audio_thread,
       scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
       AudioLogFactory* audio_log_factory);
 
   // A convenience wrapper of AudioManager::Create for testing.
-  // The given |task_runner| is shared for both audio io and heavyweight tasks.
-  static ScopedAudioManagerPtr CreateForTesting(
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+  static std::unique_ptr<AudioManager> CreateForTesting(
+      std::unique_ptr<AudioThread> audio_thread);
 
   // Starts monitoring AudioManager task runner for hangs.
   // Runs the monitor on the given |task_runner|, which must be different from
@@ -95,6 +88,11 @@ class MEDIA_EXPORT AudioManager {
   // like src/chrome.
   static AudioManager* Get();
 
+  // Releases all audio resources.
+  // Must be called before deletion and on the same thread as AudioManager
+  // was created.
+  void Shutdown();
+
   // Returns true if the OS reports existence of audio devices. This does not
   // guarantee that the existing devices support all formats and sample rates.
   virtual bool HasAudioOutputDevices() = 0;
@@ -188,17 +186,17 @@ class MEDIA_EXPORT AudioManager {
       const LogCallback& log_callback) = 0;
 
   // Returns the task runner used for audio IO.
-  // TODO(alokp): Rename to task_runner().
   base::SingleThreadTaskRunner* GetTaskRunner() const {
-    return task_runner_.get();
+    DCHECK(audio_thread_);
+    return audio_thread_->GetTaskRunner();

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

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?

[alokp, 2017/04/28 17:31:12]

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?

[o1ka, 2017/05/02 16:16:00]

See my comment to AM::Shutdown()

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

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

   }
 
   // Heavyweight tasks should use GetWorkerTaskRunner() instead of
   // GetTaskRunner(). On most platforms they are the same, but some share the
   // UI loop with the audio IO loop.
-  // TODO(alokp): Rename to worker_task_runner().
   base::SingleThreadTaskRunner* GetWorkerTaskRunner() const {
-    return worker_task_runner_.get();
+    DCHECK(audio_thread_);
+    return audio_thread_->GetWorkerTaskRunner();

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

Same here

[alokp, 2017/04/28 17:31:12]

ditto :)

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

Done.

   }
 
   // Allows clients to listen for device state changes; e.g. preferred sample
@@ -263,21 +261,19 @@ class MEDIA_EXPORT AudioManager {
  protected:
   FRIEND_TEST_ALL_PREFIXES(AudioManagerTest, AudioDebugRecording);
 
-  AudioManager(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-               scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner);
-  virtual ~AudioManager();
+  explicit AudioManager(std::unique_ptr<AudioThread> audio_thread);
 
   // Initializes output debug recording. Can be called on any thread; will post
   // to the audio thread if not called on it.
   virtual void InitializeOutputDebugRecording(
       scoped_refptr<base::SingleThreadTaskRunner> file_task_runner) = 0;
 
+  virtual void ShutdownOnAudioThread() = 0;
+
  private:
-  friend class base::DeleteHelper<AudioManager>;
-  friend class AudioManagerDeleter;
+  base::ThreadChecker thread_checker_;
+  std::unique_ptr<AudioThread> audio_thread_;
 
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-  scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner_;
   DISALLOW_COPY_AND_ASSIGN(AudioManager);
 };