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..69d9405fe6beb52f8dac9c78594dd7b46acd32a1 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,7 +217,7 @@ 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
@@ -233,9 +234,10 @@ 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, but otherwise
+// operates in Device Thread. Notably, its |suspend_observer_| lives in UI
+// thread as well; this observer calls back to indicate that there has been a
+// device change of some kind.
 class SuspendObserverDelegate :
     public base::RefCountedThreadSafe<SuspendObserverDelegate> {
  public:
@@ -251,12 +253,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 +271,9 @@ void SuspendObserverDelegate::OnDeviceChanged() {
   NSArray* devices = [AVCaptureDeviceGlue devices];
   std::vector<DeviceInfo> snapshot_devices;
   for (CrAVCaptureDevice* device in devices) {
-    [suspend_observer_ startObserving:device];
+    //[suspend_observer_ startObserving:device];
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::BindBlock(^{ [suspend_observer_ startObserving:device]; }));
     BOOL suspended = [device respondsToSelector:@selector(isSuspended)] &&
         [device isSuspended];
     DeviceInfo::DeviceType device_type = DeviceInfo::kUnknown;
@@ -290,10 +297,15 @@ 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];
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+    base::BindBlock(^{

[Robert Sesek, 2014/07/02 19:44:45]

nit: indent 2 more spaces

[mcasas, 2014/07/03 12:03:25]

Done.

+        suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc]

[Robert Sesek, 2014/07/02 19:44:45]

This seems like questionably unsafe data access.

[mcasas, 2014/07/03 12:03:25]

I'm not sure I understand. Isn't it the same as what was
done before, carried out in UI thread?

+                                    initWithChangeReceiver:this]); }));

[Robert Sesek, 2014/07/02 19:44:45]

Closing brace for the block should go on the next line if it doesn't all fit on
one line.

[mcasas, 2014/07/03 12:03:25]

Done.

+  for (CrAVCaptureDevice* device in [AVCaptureDeviceGlue devices]) {
+    //[suspend_observer_ startObserving:device];
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+        base::BindBlock(^{ [suspend_observer_ startObserving:device]; }));
+  }
 }
 
 void SuspendObserverDelegate::ResetDeviceMonitorOnUIThread() {
@@ -385,6 +397,7 @@ void AVFoundationMonitorImpl::OnDeviceChanged() {
 @implementation CrAVFoundationDeviceObserver
 
 - (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if ((self = [super init])) {
     DCHECK(receiver != NULL);
     receiver_ = receiver;
@@ -393,13 +406,17 @@ void AVFoundationMonitorImpl::OnDeviceChanged() {
 }
 
 - (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,26 +435,30 @@ 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 {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if ([keyPath isEqual:@"suspended"])
     receiver_->OnDeviceChanged();
   if ([keyPath isEqual:@"connected"])