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..13fd8f30bebcb0f080ea9c52a12412111edb6375 100644
--- a/content/browser/device_monitor_mac.mm
+++ b/content/browser/device_monitor_mac.mm
@@ -11,6 +11,7 @@
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/mac/scoped_nsobject.h"
+#include "base/threading/thread_checker.h"
 #import "media/video/capture/mac/avfoundation_glue.h"
 
 namespace {
@@ -208,22 +209,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 +232,70 @@ 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();
+ private:
+  virtual ~SuspendObserverDelegate() {}
+
+  friend class base::RefCountedThreadSafe<SuspendObserverDelegate>;
+
+  base::ThreadChecker thread_checker_;
+  base::scoped_nsobject<CrAVFoundationDeviceObserver> suspend_observer_;
+  DeviceMonitorMacImpl* avfoundation_monitor_impl_;

[tommi (sloooow) - chröme, 2014/05/13 14:43:41]

I don't see where this variable is NULLed.  It needs to be set to NULL when the
DeviceMonitorMacImpl is deleted and that needs to happen in a thread safe way to
make sure the pointer isn't being used on the device thread.

[mcasas, 2014/05/14 12:07:16]

Done.

+};
+
+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.
+  base::MessageLoopForUI::current()->PostTask(FROM_HERE,

[tommi (sloooow) - chröme, 2014/05/13 14:43:41]

base::MessageLoopForUI::current() doesn't return the UI thread loop.  It returns
the current message loop and DCHECKs that the type of the current loop is
TYPE_UI.  See here:

https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...

[mcasas, 2014/05/14 12:07:16]

You're totally right. I meant using:
BrowserThread::PostTask(BrowserThread::UI, ...)

+      base::Bind(&DeviceMonitorMacImpl::ConsolidateDevicesListAndNotify,
+          base::Unretained(avfoundation_monitor_impl_), snapshot_devices));
+}
+
+void SuspendObserverDelegate::StartObserver() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc]
+                               initWithChangeReceiver:this]);
+  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices])
+    [suspend_observer_ startObserving:device];
+}
+
 // 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 |deivce_task_runner_|

[tommi (sloooow) - chröme, 2014/05/13 14:43:41]

device_task_runner_

[mcasas, 2014/05/14 12:07:16]

Done.

+// 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 +306,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 +322,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 +340,12 @@ 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());

[tommi (sloooow) - chröme, 2014/05/13 14:43:41]

here you would need to do something like:

suspend_observer_delegate_->ResetDeviceMonitor();

to make sure the object on the device thread never pulls a use-after-free.

[mcasas, 2014/05/14 12:07:16]

Done.

   NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
   [nc removeObserver:device_arrival_];
   [nc removeObserver:device_removal_];
@@ -295,52 +354,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;