DeviceMonitorMac AVFoundation: move mgmt of |suspend_observer_| to a class living in Device Thread.

content/browser/device_monitor_mac.mm

Index: content/browser/device_monitor_mac.mm
diff --git a/content/browser/device_monitor_mac.mm b/content/browser/device_monitor_mac.mm
index ae720cbdd9686017b2b934438eb6af6e00e1b8f5..97abd41117aa07e48d0764d9c3b5daea88fc5efe 100644
--- a/content/browser/device_monitor_mac.mm
+++ b/content/browser/device_monitor_mac.mm
@@ -11,6 +11,9 @@
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/mac/scoped_nsobject.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
+#include "content/public/browser/browser_thread.h"
 #import "media/video/capture/mac/avfoundation_glue.h"
 
 namespace {
@@ -208,22 +211,22 @@ void QTKitMonitorImpl::OnDeviceChanged() {
 }
 
 // Forward declaration for use by CrAVFoundationDeviceObserver.
-class AVFoundationMonitorImpl;
+class SuspendObserverDelegate;
 
 }  // namespace
 
 // This class is a Key-Value Observer (KVO) shim. It is needed because C++
-// classes cannot observe Key-Values directly. This class is used by
-// AVfoundationMonitorImpl and executed in its |device_task_runner_|, a.k.a.
-// "Device Thread". -stopObserving is called dutifully on -dealloc on UI thread.
+// classes cannot observe Key-Values directly. This class is used and
+// manipulated by SuspendObserverDelegate in Device Thread. -stopObserving is
+// called dutifully on -dealloc on Device Thread too.
 @interface CrAVFoundationDeviceObserver : NSObject {
  @private
-  AVFoundationMonitorImpl* receiver_;
+  SuspendObserverDelegate* receiver_;
   // Member to keep track of the devices we are already monitoring.
   std::set<CrAVCaptureDevice*> monitoredDevices_;
 }
 
-- (id)initWithChangeReceiver:(AVFoundationMonitorImpl*)receiver;
+- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver;
 - (void)startObserving:(CrAVCaptureDevice*)device;
 - (void)stopObserving:(CrAVCaptureDevice*)device;
 
@@ -231,10 +234,86 @@ class AVFoundationMonitorImpl;
 
 namespace {
 
+// This class owns and manages the lifetime of a CrAVFoundationDeviceObserver.
+// Provides a callback for this device observer to indicate that there has been
+// a device change of some kind. Created by AVFoundationMonitorImpl in UI thread
+// but living in Device Thread.
+class SuspendObserverDelegate :
+    public base::RefCountedThreadSafe<SuspendObserverDelegate> {
+ public:
+  SuspendObserverDelegate(DeviceMonitorMacImpl* monitor)
+      : avfoundation_monitor_impl_(monitor) {
+    thread_checker_.DetachFromThread();
+  }
+
+  void OnDeviceChanged();
+  void StartObserver();
+  void ResetDeviceMonitorOnUIThread();
+
+ private:
+  virtual ~SuspendObserverDelegate() {}
+
+  friend class base::RefCountedThreadSafe<SuspendObserverDelegate>;
+
+  base::ThreadChecker thread_checker_;
+  base::scoped_nsobject<CrAVFoundationDeviceObserver> suspend_observer_;
+  // |lock_| protects concurrent usage + reset of |avfoundation_monitor_impl_|.
+  base::Lock lock_;
+  DeviceMonitorMacImpl* avfoundation_monitor_impl_;
+};
+
+void SuspendObserverDelegate::OnDeviceChanged() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  NSArray* devices = [AVCaptureDeviceGlue devices];
+  std::vector<DeviceInfo> snapshot_devices;
+  for (CrAVCaptureDevice* device in devices) {
+    [suspend_observer_ startObserving:device];
+    BOOL suspended = [device respondsToSelector:@selector(isSuspended)] &&
+        [device isSuspended];
+    DeviceInfo::DeviceType device_type = DeviceInfo::kUnknown;
+    if ([device hasMediaType:AVFoundationGlue::AVMediaTypeVideo()]) {
+      if (suspended)
+        continue;
+      device_type = DeviceInfo::kVideo;
+    } else if ([device hasMediaType:AVFoundationGlue::AVMediaTypeMuxed()]) {
+      device_type = suspended ? DeviceInfo::kAudio : DeviceInfo::kMuxed;
+    } else if ([device hasMediaType:AVFoundationGlue::AVMediaTypeAudio()]) {
+      device_type = DeviceInfo::kAudio;
+    }
+    snapshot_devices.push_back(DeviceInfo([[device uniqueID] UTF8String],
+                                          device_type));
+  }
+  // Posts the consolidation of enumerated devices to be done on UI thread. This
+  // has to happen in mutex with ResetDeviceMonitorOnUIThread().
+  {
+    base::AutoLock auto_lock(lock_);
+    if (avfoundation_monitor_impl_) {
+      content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+          base::Bind(&DeviceMonitorMacImpl::ConsolidateDevicesListAndNotify,
+              base::Unretained(avfoundation_monitor_impl_), snapshot_devices));

[tommi (sloooow) - chröme, 2014/05/15 13:35:39]

This is unfortunately not safe either.  The pointer can be non-NULL here but
when the task runs, it can be NULL.

Here's another proposal:

Since SuspendObserverDelegate is reference counted, I propose you post a task to
|this| (*not* using base::Unretained).

In the function that gets called back on the UI thread, DCHECK(<ui thread>),
check avfoundation_monitor_impl_ for being NULL and in the case of it not being
NULL, call DeviceMonitorMacImpl::ConsolidateDevicesListAndNotify synchronously.

Once that's done, you can remove the |lock_| member variable.

[mcasas, 2014/05/15 13:52:19]

Good suggestion. Done.

+    }
+  }
+}
+
+void SuspendObserverDelegate::StartObserver() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc]
+                               initWithChangeReceiver:this]);
+  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices])
+    [suspend_observer_ startObserving:device];
+}
+
+void SuspendObserverDelegate::ResetDeviceMonitorOnUIThread() {
+  DCHECK(base::MessageLoopForUI::IsCurrent());
+  base::AutoLock auto_lock(lock_);
+  avfoundation_monitor_impl_ = NULL;
+}
+
 // AVFoundation implementation of the Mac Device Monitor, registers as a global
 // device connect/disconnect observer and plugs suspend/wake up device observers
-// per device. This class is created and lives in UI thread; device enumeration
-// and operations involving |suspend_observer_| happen on |device_task_runner_|.
+// per device. Owns a SuspendObserverDelegate living in |device_task_runner_|
+// and gets notified when a device is suspended/resumed. This class is created
+// and lives in UI thread;
 class AVFoundationMonitorImpl : public DeviceMonitorMacImpl {
  public:
   AVFoundationMonitorImpl(
@@ -245,18 +324,14 @@ class AVFoundationMonitorImpl : public DeviceMonitorMacImpl {
   virtual void OnDeviceChanged() OVERRIDE;
 
  private:
-  void OnDeviceChangedOnDeviceThread(
-      const scoped_refptr<base::MessageLoopProxy>& ui_thread);
-  void StartObserverOnDeviceThread();
-
   base::ThreadChecker thread_checker_;
 
   // {Video,AudioInput}DeviceManager's "Device" thread task runner used for
-  // device enumeration, valid after MediaStreamManager calls StartMonitoring().
+  // posting tasks to |suspend_observer_delegate_|; valid after
+  // MediaStreamManager calls StartMonitoring().
   const scoped_refptr<base::SingleThreadTaskRunner> device_task_runner_;
 
-  // Created and executed in |device_task_runnner_|.
-  base::scoped_nsobject<CrAVFoundationDeviceObserver> suspend_observer_;
+  scoped_refptr<SuspendObserverDelegate> suspend_observer_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(AVFoundationMonitorImpl);
 };
@@ -265,7 +340,8 @@ AVFoundationMonitorImpl::AVFoundationMonitorImpl(
     content::DeviceMonitorMac* monitor,
     const scoped_refptr<base::SingleThreadTaskRunner>& device_task_runner)
     : DeviceMonitorMacImpl(monitor),
-      device_task_runner_(device_task_runner) {
+      device_task_runner_(device_task_runner),
+      suspend_observer_delegate_(new SuspendObserverDelegate(this)) {
   NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
   device_arrival_ =
       [nc addObserverForName:AVFoundationGlue::
@@ -282,11 +358,13 @@ AVFoundationMonitorImpl::AVFoundationMonitorImpl(
                   usingBlock:^(NSNotification* notification) {
                       OnDeviceChanged();}];
   device_task_runner_->PostTask(FROM_HERE,
-      base::Bind(&AVFoundationMonitorImpl::StartObserverOnDeviceThread,
-          base::Unretained(this)));
+      base::Bind(&SuspendObserverDelegate::StartObserver,
+                 suspend_observer_delegate_));
 }
 
 AVFoundationMonitorImpl::~AVFoundationMonitorImpl() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  suspend_observer_delegate_->ResetDeviceMonitorOnUIThread();
   NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
   [nc removeObserver:device_arrival_];
   [nc removeObserver:device_removal_];
@@ -295,52 +373,15 @@ AVFoundationMonitorImpl::~AVFoundationMonitorImpl() {
 void AVFoundationMonitorImpl::OnDeviceChanged() {
   DCHECK(thread_checker_.CalledOnValidThread());
   device_task_runner_->PostTask(FROM_HERE,
-      base::Bind(&AVFoundationMonitorImpl::OnDeviceChangedOnDeviceThread,
-          base::Unretained(this),
-          base::MessageLoop::current()->message_loop_proxy()));
-}
-
-void AVFoundationMonitorImpl::OnDeviceChangedOnDeviceThread(
-    const scoped_refptr<base::MessageLoopProxy>& ui_thread) {
-  DCHECK(device_task_runner_->BelongsToCurrentThread());
-  NSArray* devices = [AVCaptureDeviceGlue devices];
-  std::vector<DeviceInfo> snapshot_devices;
-  for (CrAVCaptureDevice* device in devices) {
-    [suspend_observer_ startObserving:device];
-    BOOL suspended = [device respondsToSelector:@selector(isSuspended)] &&
-        [device isSuspended];
-    DeviceInfo::DeviceType device_type = DeviceInfo::kUnknown;
-    if ([device hasMediaType:AVFoundationGlue::AVMediaTypeVideo()]) {
-      if (suspended)
-        continue;
-      device_type = DeviceInfo::kVideo;
-    } else if ([device hasMediaType:AVFoundationGlue::AVMediaTypeMuxed()]) {
-      device_type = suspended ? DeviceInfo::kAudio : DeviceInfo::kMuxed;
-    } else if ([device hasMediaType:AVFoundationGlue::AVMediaTypeAudio()]) {
-      device_type = DeviceInfo::kAudio;
-    }
-    snapshot_devices.push_back(DeviceInfo([[device uniqueID] UTF8String],
-                                          device_type));
-  }
-  // Post the consolidation of enumerated devices to be done on UI thread.
-  ui_thread->PostTask(FROM_HERE,
-      base::Bind(&DeviceMonitorMacImpl::ConsolidateDevicesListAndNotify,
-          base::Unretained(this), snapshot_devices));
-}
-
-void AVFoundationMonitorImpl::StartObserverOnDeviceThread() {
-  DCHECK(device_task_runner_->BelongsToCurrentThread());
-  suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc]
-                               initWithChangeReceiver:this]);
-  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices])
-    [suspend_observer_ startObserving:device];
+      base::Bind(&SuspendObserverDelegate::OnDeviceChanged,
+                 suspend_observer_delegate_));
 }
 
 }  // namespace
 
 @implementation CrAVFoundationDeviceObserver
 
-- (id)initWithChangeReceiver:(AVFoundationMonitorImpl*)receiver {
+- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver {
   if ((self = [super init])) {
     DCHECK(receiver != NULL);
     receiver_ = receiver;