DeviceMonitorMac: move CrAVFoundationDeviceObserver and most of SuspendObserverDelegate to UI Thread

content/browser/device_monitor_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/device_monitor_mac.h"
 
 #import <QTKit/QTKit.h>
 
 #include <set>
 
 #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"
 #import "media/video/capture/mac/avfoundation_glue.h"
 
 namespace {
 
 // This class is used to keep track of system devices names and their types.
 class DeviceInfo {
  public:
@@ skipping 186 matching lines @@
   ConsolidateDevicesListAndNotify(snapshot_devices);
 }
 
 // Forward declaration for use by CrAVFoundationDeviceObserver.
 class SuspendObserverDelegate;
 
 }  // namespace
 
 // 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;
 
 @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.
+// 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();
   }
 
   void OnDeviceChanged();
   void StartObserver();
   void ResetDeviceMonitorOnUIThread();
 
  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_;
 };
 
 void SuspendObserverDelegate::OnDeviceChanged() {
   DCHECK(device_thread_checker_.CalledOnValidThread());
   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;
     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.
   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];
+  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() {
   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));
@@ skipping 70 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   device_task_runner_->PostTask(FROM_HERE,
       base::Bind(&SuspendObserverDelegate::OnDeviceChanged,
                  suspend_observer_delegate_));
 }
 
 }  // namespace
 
 @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) !=
           monitoredDevices_.end()) {
     return;
   }
   [device addObserver:self
            forKeyPath:@"suspended"
               options:0
               context:device];
   [device addObserver:self
            forKeyPath:@"connected"
               options:0
               context:device];
   monitoredDevices_.insert(device);
 }
 
 - (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)];
 }
 
 @end  // @implementation CrAVFoundationDeviceObserver
 
 namespace content {
 
 DeviceMonitorMac::DeviceMonitorMac() {
   // Both QTKit and AVFoundation do not need to be fired up until the user
@@ skipping 18 matching lines @@
 }
 
 void DeviceMonitorMac::NotifyDeviceChanged(
     base::SystemMonitor::DeviceType type) {
   DCHECK(thread_checker_.CalledOnValidThread());
   // TODO(xians): Remove the global variable for SystemMonitor.
   base::SystemMonitor::Get()->ProcessDevicesChanged(type);
 }
 
 }  // namespace content