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..f4ba79c08593e4d5ef6fbda2ffdf2e1aeefa9b81 100644
--- a/content/browser/device_monitor_mac.mm
+++ b/content/browser/device_monitor_mac.mm
@@ -10,6 +10,7 @@
 
 #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"
@@ -216,15 +217,17 @@ 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
+  scoped_refptr<base::MessageLoopProxy> device_thread_message_loop_;
+  scoped_refptr<SuspendObserverDelegate> receiver_;

[tommi (sloooow) - chröme, 2014/07/03 13:30:17]

instead of holding on to the loop + receiver, what about changing this to use a
callback object?  If the caller wants the callback to happen on another thread
(e.g. the device thread), that detail doesn't have to be contained in
CrAVFoundationDeviceObserver.  The caller can take care of that when building
the callback object itself.

[mcasas, 2014/07/04 14:08:58]

That makes sense. Will do.

   // Member to keep track of the devices we are already monitoring.
   std::set<CrAVCaptureDevice*> monitoredDevices_;
 }
 
-- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver;
+- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver
+           deviceMessageLoop:(scoped_refptr<base::MessageLoopProxy>)loop;
 - (void)startObserving:(CrAVCaptureDevice*)device;
 - (void)stopObserving:(CrAVCaptureDevice*)device;
 
@@ -233,14 +236,15 @@ class SuspendObserverDelegate;
 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.
+// AVFoundationMonitorImpl creates and destroys it in UI thread. Runs the
+// expensive device enumerations in OnDeviceChanged() and StartObserver() on
+// Device Thread.
 class SuspendObserverDelegate :
     public base::RefCountedThreadSafe<SuspendObserverDelegate> {
  public:
   explicit SuspendObserverDelegate(DeviceMonitorMacImpl* monitor)
       : avfoundation_monitor_impl_(monitor) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
     device_thread_checker_.DetachFromThread();
   }
 
@@ -251,12 +255,15 @@ class SuspendObserverDelegate :
  private:
   friend class base::RefCountedThreadSafe<SuspendObserverDelegate>;
 
-  virtual ~SuspendObserverDelegate() {}
+  virtual ~SuspendObserverDelegate() {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  }
 
   void OnDeviceChangedOnUIThread(
       const std::vector<DeviceInfo>& snapshot_devices);
 
   base::ThreadChecker device_thread_checker_;
+  // Created, used and released in UI thread.
   base::scoped_nsobject<CrAVFoundationDeviceObserver> suspend_observer_;
   DeviceMonitorMacImpl* avfoundation_monitor_impl_;
 };
@@ -266,7 +273,8 @@ void SuspendObserverDelegate::OnDeviceChanged() {
   NSArray* devices = [AVCaptureDeviceGlue devices];
   std::vector<DeviceInfo> snapshot_devices;
   for (CrAVCaptureDevice* device in devices) {
-    [suspend_observer_ startObserving:device];
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::BindBlock(^{ [suspend_observer_ startObserving:device]; }));

[tommi (sloooow) - chröme, 2014/07/03 13:30:17]

does this also addref the device pointer?

If not, we may have a race since the reference is then (I presume) only held by
the |devices| array which can be modified on the device thread.

[mcasas, 2014/07/04 14:08:58]

|device| is a naked pointer, comes verbatim from the enumeration
snapshot and is tossed to |suspend_observer_| in UI thread, what'd 
be the race then? Is not used here (Device thread) otherwise.

Or do you mean |device| might get invalidated between enumeration
and observer registration...?

[tommi (sloooow) - chröme, 2014/07/07 07:41:07]

Yes.  If you think about the case where refcount is 1 here, we post, device gets
removed (ref==0), the task executes -> use-after-free.
To make this safe, the device pointer needs to be add-refed while the task
lives.

[mcasas, 2014/07/08 15:11:45]

We tackled this topic offline and, basically, solving this race means
reference counting in several places, in particular between the 
enumerated |devices| and the CrAVfoundationDeviceObserver
usage of them.

     BOOL suspended = [device respondsToSelector:@selector(isSuspended)] &&
         [device isSuspended];
     DeviceInfo::DeviceType device_type = DeviceInfo::kUnknown;
@@ -290,10 +298,18 @@ void SuspendObserverDelegate::OnDeviceChanged() {
 
 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];
+  scoped_refptr<base::MessageLoopProxy> device_message_loop =
+      base::MessageLoopProxy::current();
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::BindBlock(^{ suspend_observer_.reset(
+          [[CrAVFoundationDeviceObserver alloc]
+              initWithChangeReceiver:this
+                   deviceMessageLoop:device_message_loop]);

[tommi (sloooow) - chröme, 2014/07/03 13:30:17]

Is passing the device_message_loop this way to the bound block, another way of
just doing PostTaskAndReply? (or is there more to it?)

[mcasas, 2014/07/04 14:08:58]

|device_message_loop| is cached inside CrAVFoundationDeviceObserver
and used repeatedly (unless changed to callback like you propose earlier).

+      }));
+  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices]) {
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::BindBlock(^{ [suspend_observer_ startObserving:device]; }));

[tommi (sloooow) - chröme, 2014/07/03 13:30:17]

seems like we're posting many tasks to the UI thread in this function.  Would it
make sense to just post a single task that does all these things?

[mcasas, 2014/07/04 14:08:58]

l.309 ought to be executed in Device Thread, is the time consuming
device enumeration.

+  }
 }
 
 void SuspendObserverDelegate::ResetDeviceMonitorOnUIThread() {
@@ -384,22 +400,29 @@ void AVFoundationMonitorImpl::OnDeviceChanged() {
 
 @implementation CrAVFoundationDeviceObserver
 
-- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver {
+- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver
+           deviceMessageLoop:(scoped_refptr<base::MessageLoopProxy>)loop {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if ((self = [super init])) {
     DCHECK(receiver != NULL);
     receiver_ = receiver;
+    device_thread_message_loop_ = loop;
   }
   return self;
 }
 
 - (void)dealloc {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   std::set<CrAVCaptureDevice*>::iterator it = monitoredDevices_.begin();
-  while (it != monitoredDevices_.end())
-    [self stopObserving:*it++];
+  while (it != monitoredDevices_.end()) {
+    [self removeObservers:*it];
+    monitoredDevices_.erase(it++);
+  }
   [super dealloc];
 }
 
 - (void)startObserving:(CrAVCaptureDevice*)device {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(device != nil);
   // Skip this device if there are already observers connected to it.
   if (std::find(monitoredDevices_.begin(), monitoredDevices_.end(), device) !=
@@ -418,28 +441,34 @@ void AVFoundationMonitorImpl::OnDeviceChanged() {
 }
 
 - (void)stopObserving:(CrAVCaptureDevice*)device {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK(device != nil);
   std::set<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)removeObservers:(CrAVCaptureDevice*)device {
+  // 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 {
-  if ([keyPath isEqual:@"suspended"])
-    receiver_->OnDeviceChanged();
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  if ([keyPath isEqual:@"suspended"]) {
+    device_thread_message_loop_->PostTask(FROM_HERE,
+        base::Bind(&SuspendObserverDelegate::OnDeviceChanged, receiver_));
+  }
   if ([keyPath isEqual:@"connected"])
     [self stopObserving:static_cast<CrAVCaptureDevice*>(context)];
 }