DeviceMonitorMac: move CrAVFoundationDeviceObserver and most of SuspendObserverDelegate to UI 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 eb429afe6402bb07bfc3f100ec31e9c05e2665e9..cdb28fe915172201abdba1213abab80e659314cc 100644
--- a/content/browser/device_monitor_mac.mm
+++ b/content/browser/device_monitor_mac.mm
@@ -10,11 +10,15 @@
 
 #include "base/bind_helpers.h"
 #include "base/logging.h"
+#include "base/mac/bind_objc_block.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/threading/thread_checker.h"
 #include "content/public/browser/browser_thread.h"
+#include "media/base/bind_to_current_loop.h"
 #import "media/video/capture/mac/avfoundation_glue.h"
 
+using content::BrowserThread;
+
 namespace {
 
 // This class is used to keep track of system devices names and their types.
@@ -216,57 +220,107 @@ class SuspendObserverDelegate;
 
 // This class is a Key-Value Observer (KVO) shim. It is needed because C++
 // classes cannot observe Key-Values directly. Created, manipulated, and
-// destroyed on the Device Thread by SuspendedObserverDelegate.
+// destroyed on the UI Thread by SuspendObserverDelegate.
 @interface CrAVFoundationDeviceObserver : NSObject {
  @private
-  SuspendObserverDelegate* receiver_;  // weak
+  // Callback for device changed, has to run on Device Thread.
+  base::Closure onDeviceChangedCallback_;
+
   // Member to keep track of the devices we are already monitoring.
-  std::set<CrAVCaptureDevice*> monitoredDevices_;
+  std::set<base::scoped_nsobject<CrAVCaptureDevice> > monitoredDevices_;
 }
 
-- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver;
-- (void)startObserving:(CrAVCaptureDevice*)device;
-- (void)stopObserving:(CrAVCaptureDevice*)device;
+- (id)initWithOnChangedCallback:(const base::Closure&)callback;
+- (void)startObserving:(base::scoped_nsobject<CrAVCaptureDevice>)device;
+- (void)stopObserving:(const CrAVCaptureDevice*)device;
+- (void)clearOnDeviceChangedCallback;
 
 @end
 
 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.
+// It is created and destroyed in UI thread by AVFoundationMonitorImpl, and it
+// operates in this thread except for the expensive device enumerations which
+// are run on Device Thread.
 class SuspendObserverDelegate :
     public base::RefCountedThreadSafe<SuspendObserverDelegate> {
  public:
-  explicit SuspendObserverDelegate(DeviceMonitorMacImpl* monitor)
-      : avfoundation_monitor_impl_(monitor) {
-    device_thread_checker_.DetachFromThread();
-  }
+  explicit SuspendObserverDelegate(DeviceMonitorMacImpl* monitor);
 
-  void OnDeviceChanged();
-  void StartObserver();
-  void ResetDeviceMonitorOnUIThread();
+  void StartObserver(

[Robert Sesek, 2014/07/08 17:05:40]

Comment these please.

[mcasas, 2014/07/09 08:27:33]

Done.

+      const scoped_refptr<base::SingleThreadTaskRunner>& device_thread);
+  void OnDeviceChanged(
+      const scoped_refptr<base::SingleThreadTaskRunner>& device_thread);
+  void ResetDeviceMonitor();
 
  private:
   friend class base::RefCountedThreadSafe<SuspendObserverDelegate>;
 
-  virtual ~SuspendObserverDelegate() {}
+  virtual ~SuspendObserverDelegate();
 
-  void OnDeviceChangedOnUIThread(
-      const std::vector<DeviceInfo>& snapshot_devices);
+  void DoStartObserver(NSArray* devices);

[Robert Sesek, 2014/07/08 17:05:40]

Comment about ownership of |devices| here, after you fix the leaks.

[tommi (sloooow) - chröme, 2014/07/08 17:23:42]

Does scoped_nsobject<> support this sort of scenario?  i.e. could this be:

void DoStartObserver(const scoped_nsobject<NSArray>& devices);

[Robert Sesek, 2014/07/08 17:25:25]

It's pretty atypical I think to see things passed around as scoped_nsobject<T>
in params. Typically the callee simply retains the object if it wants to ensure
its lifetime.

[tommi (sloooow) - chröme, 2014/07/08 17:29:29]

OK, what I was after was basically the same pattern as we use elsewhere with
scoped_refptr<>.
If that's going against the convention then explicit refcounting is fine with
me.

[mcasas, 2014/07/09 08:27:33]

Added comment about DoStartObserver owning the |devices|.

|devices| is created in another thread, that's why the -retain 
has to be done there. Added [devices release] at the end
of the function (only one exit point).

[tommi (sloooow) - chröme, 2014/07/09 09:19:39]

Since DoStartObserver releases the reference of |devices|, I think it's worth
documenting that explicitly for this method.

+  void DoOnDeviceChanged(NSArray* devices);
 
-  base::ThreadChecker device_thread_checker_;
   base::scoped_nsobject<CrAVFoundationDeviceObserver> suspend_observer_;
   DeviceMonitorMacImpl* avfoundation_monitor_impl_;
 };
 
-void SuspendObserverDelegate::OnDeviceChanged() {
-  DCHECK(device_thread_checker_.CalledOnValidThread());
-  NSArray* devices = [AVCaptureDeviceGlue devices];
+SuspendObserverDelegate::SuspendObserverDelegate(DeviceMonitorMacImpl* monitor)
+    : avfoundation_monitor_impl_(monitor) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+void SuspendObserverDelegate::StartObserver(
+      const scoped_refptr<base::SingleThreadTaskRunner>& device_thread) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  base::Closure on_device_changed_callback =
+      base::Bind(&SuspendObserverDelegate::OnDeviceChanged,
+                 this, device_thread);
+  suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc]
+      initWithOnChangedCallback:on_device_changed_callback]);
+
+  base::PostTaskAndReplyWithResult(device_thread, FROM_HERE,
+    base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }),

[Robert Sesek, 2014/07/08 17:05:40]

nit: indent 2 more spaces, same with the next line

[tommi (sloooow) - chröme, 2014/07/08 17:23:42]

use scoped_nsobject wherever possible to reduce the likelihood of leaks like
this.

[mcasas, 2014/07/09 08:27:33]

As per the discussion above,  I'll avoid using
scoped_nsobject<T> in params, but add some info and
the -release call.

[mcasas, 2014/07/09 08:27:33]

Done.

+    base::Bind(&SuspendObserverDelegate::DoStartObserver, this));
+}
+
+void SuspendObserverDelegate::OnDeviceChanged(
+      const scoped_refptr<base::SingleThreadTaskRunner>& device_thread) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Enumerate the devices in Device thread and post the consolidation of
+  // the devices and the old ones to be done on UI thread.
+  PostTaskAndReplyWithResult(device_thread, FROM_HERE,
+      base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }),

[tommi (sloooow) - chröme, 2014/07/08 17:23:42]

same here

[mcasas, 2014/07/09 08:27:33]

Ditto: No scoped_nsobject<T> in params.

+      base::Bind(&SuspendObserverDelegate::DoOnDeviceChanged, this));
+}
+
+void SuspendObserverDelegate::ResetDeviceMonitor() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  avfoundation_monitor_impl_ = NULL;
+  [suspend_observer_ clearOnDeviceChangedCallback];
+}
+
+SuspendObserverDelegate::~SuspendObserverDelegate() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+void SuspendObserverDelegate::DoStartObserver(NSArray* devices) {

[Robert Sesek, 2014/07/08 17:05:40]

|devices| is going to be leaked from line 284 / 295

[mcasas, 2014/07/09 08:27:34]

Done here and inside DoOnDeviceChanged(), that also owns
the |devices| array passed as parameter.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  for (CrAVCaptureDevice* device in devices) {
+    base::scoped_nsobject<CrAVCaptureDevice> device_ptr([device retain]);

[Robert Sesek, 2014/07/08 17:05:40]

Do you need this line? The array owns the object.

[mcasas, 2014/07/09 08:27:33]

We/I need to keep an extra reference for the scoped ptr to 
release it later on.

+    [suspend_observer_ startObserving:device_ptr];
+  }
+}
+
+void SuspendObserverDelegate::DoOnDeviceChanged(NSArray* devices) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   std::vector<DeviceInfo> snapshot_devices;
   for (CrAVCaptureDevice* device in devices) {
-    [suspend_observer_ startObserving:device];
+    base::scoped_nsobject<CrAVCaptureDevice> device_ptr([device retain]);
+    [suspend_observer_ startObserving:device_ptr];
+
     BOOL suspended = [device respondsToSelector:@selector(isSuspended)] &&
         [device isSuspended];
     DeviceInfo::DeviceType device_type = DeviceInfo::kUnknown;
@@ -282,28 +336,7 @@ void SuspendObserverDelegate::OnDeviceChanged() {
     snapshot_devices.push_back(DeviceInfo([[device uniqueID] UTF8String],
                                           device_type));
   }
-  // Post the consolidation of enumerated devices to be done on UI thread.
-  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&SuspendObserverDelegate::OnDeviceChangedOnUIThread,
-          this, snapshot_devices));
-}
-
-void SuspendObserverDelegate::StartObserver() {
-  DCHECK(device_thread_checker_.CalledOnValidThread());
-  suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc]
-                               initWithChangeReceiver:this]);
-  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices])
-    [suspend_observer_ startObserving:device];
-}
 
-void SuspendObserverDelegate::ResetDeviceMonitorOnUIThread() {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  avfoundation_monitor_impl_ = NULL;
-}
-
-void SuspendObserverDelegate::OnDeviceChangedOnUIThread(
-    const std::vector<DeviceInfo>& snapshot_devices) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   // |avfoundation_monitor_impl_| might have been NULLed asynchronously before
   // arriving at this line.
   if (avfoundation_monitor_impl_) {
@@ -314,9 +347,8 @@ void SuspendObserverDelegate::OnDeviceChangedOnUIThread(
 
 // AVFoundation implementation of the Mac Device Monitor, registers as a global
 // device connect/disconnect observer and plugs suspend/wake up device observers
-// 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;
+// per device. This class is created and lives in UI thread. Owns a
+// SuspendObserverDelegate that notifies when a device is suspended/resumed.
 class AVFoundationMonitorImpl : public DeviceMonitorMacImpl {
  public:
   AVFoundationMonitorImpl(
@@ -327,8 +359,6 @@ class AVFoundationMonitorImpl : public DeviceMonitorMacImpl {
   virtual void OnDeviceChanged() OVERRIDE;
 
  private:
-  base::ThreadChecker thread_checker_;
-
   // {Video,AudioInput}DeviceManager's "Device" thread task runner used for
   // posting tasks to |suspend_observer_delegate_|; valid after
   // MediaStreamManager calls StartMonitoring().
@@ -345,6 +375,7 @@ AVFoundationMonitorImpl::AVFoundationMonitorImpl(
     : DeviceMonitorMacImpl(monitor),
       device_task_runner_(device_task_runner),
       suspend_observer_delegate_(new SuspendObserverDelegate(this)) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
   device_arrival_ =
       [nc addObserverForName:AVFoundationGlue::
@@ -360,86 +391,97 @@ AVFoundationMonitorImpl::AVFoundationMonitorImpl(
                        queue:nil
                   usingBlock:^(NSNotification* notification) {
                       OnDeviceChanged();}];
-  device_task_runner_->PostTask(FROM_HERE,
-      base::Bind(&SuspendObserverDelegate::StartObserver,
-                 suspend_observer_delegate_));
+  suspend_observer_delegate_->StartObserver(device_task_runner_);
 }
 
 AVFoundationMonitorImpl::~AVFoundationMonitorImpl() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  suspend_observer_delegate_->ResetDeviceMonitorOnUIThread();
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  suspend_observer_delegate_->ResetDeviceMonitor();
   NSNotificationCenter* nc = [NSNotificationCenter defaultCenter];
   [nc removeObserver:device_arrival_];
   [nc removeObserver:device_removal_];
 }
 
 void AVFoundationMonitorImpl::OnDeviceChanged() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  device_task_runner_->PostTask(FROM_HERE,
-      base::Bind(&SuspendObserverDelegate::OnDeviceChanged,
-                 suspend_observer_delegate_));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  suspend_observer_delegate_->OnDeviceChanged(device_task_runner_);
 }
 
 }  // namespace
 
 @implementation CrAVFoundationDeviceObserver
 
-- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver {
+- (id)initWithOnChangedCallback:(const base::Closure&)callback {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if ((self = [super init])) {
-    DCHECK(receiver != NULL);
-    receiver_ = receiver;
+    DCHECK(!callback.is_null());
+    onDeviceChangedCallback_ = callback;
   }
   return self;
 }
 
 - (void)dealloc {
-  std::set<CrAVCaptureDevice*>::iterator it = monitoredDevices_.begin();
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  std::set<base::scoped_nsobject<CrAVCaptureDevice> >::iterator it =
+      monitoredDevices_.begin();
   while (it != monitoredDevices_.end())
-    [self stopObserving:*it++];
+    [self removeObservers:*(it++)];
   [super dealloc];
 }
 
-- (void)startObserving:(CrAVCaptureDevice*)device {
+- (void)startObserving:(base::scoped_nsobject<CrAVCaptureDevice>)device {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(device != nil);
   // Skip this device if there are already observers connected to it.
   if (std::find(monitoredDevices_.begin(), monitoredDevices_.end(), device) !=
-          monitoredDevices_.end()) {
+      monitoredDevices_.end()) {
     return;
   }
   [device addObserver:self
            forKeyPath:@"suspended"
               options:0
-              context:device];
+              context:device.get()];
   [device addObserver:self
            forKeyPath:@"connected"
               options:0
-              context:device];
+              context:device.get()];
   monitoredDevices_.insert(device);
 }
 
-- (void)stopObserving:(CrAVCaptureDevice*)device {
+- (void)stopObserving:(const CrAVCaptureDevice*)device {

[Robert Sesek, 2014/07/08 17:05:40]

ObjC objects are never const.

[mcasas, 2014/07/09 08:27:33]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(device != nil);
-  std::set<CrAVCaptureDevice*>::iterator found =
+
+  std::set<base::scoped_nsobject<CrAVCaptureDevice> >::iterator found =
       std::find(monitoredDevices_.begin(), monitoredDevices_.end(), device);
   DCHECK(found != monitoredDevices_.end());
-  // Every so seldom, |device| might be gone when getting here, in that case
-  // removing the observer causes a crash. Try to avoid it by checking sanity of
-  // the |device| via its -observationInfo. http://crbug.com/371271.
+  [self removeObservers:*found];
+  monitoredDevices_.erase(found);
+}
+
+- (void)clearOnDeviceChangedCallback {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  onDeviceChangedCallback_.Reset();
+}
+
+- (void)removeObservers:(const CrAVCaptureDevice*)device {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Check sanity of |device| via its -observationInfo. http://crbug.com/371271.
   if ([device observationInfo]) {
     [device removeObserver:self
                 forKeyPath:@"suspended"];
     [device removeObserver:self
                 forKeyPath:@"connected"];
   }
-  monitoredDevices_.erase(found);
 }
 
 - (void)observeValueForKeyPath:(NSString*)keyPath
                       ofObject:(id)object
                         change:(NSDictionary*)change
                        context:(void*)context {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if ([keyPath isEqual:@"suspended"])
-    receiver_->OnDeviceChanged();
+    onDeviceChangedCallback_.Run();
   if ([keyPath isEqual:@"connected"])
     [self stopObserving:static_cast<CrAVCaptureDevice*>(context)];
 }