From a0188671ebf998dd14c46b1be0e1bbb817dd0480 Mon Sep 17 00:00:00 2001 From: "mcasas@chromium.org" Date: Fri, 11 Jul 2014 22:56:39 +0000 Subject: [PATCH] DeviceMonitorMac: move CrAVFoundationDeviceObserver and most of SuspendObserverDelegate to UI Thread (tl; dr : Move all operations to UI thread except device enumerations.) CrAVFoundationObserver was located in Device Thread based on the assumption that OS KVO callbacks would come on that thread too, but instead they come from UI thread. -observeValueForKeyPath:... is then called in UI thread, and since the rest of the actions of the class are small, this CL moves the whole class to UI thread. Its overlord SuspendObserverDelegate (best not use acronyms for its name :) ), however, lives a mixed life in UI and Device threads. The model is simplified by making it work always in UI thread _except_ for device enumerations (done via [AVCaptureDeviceglue devices]). AVFoundationMonitorImpl will destroy SuspendObserverDelegate in UI thread and that in turn destroys CrAVFoundationObserver in that very thread, thus cleaning up the multi threading and hopefully addressing the bug reports of a small but consistent crash rate (~2 crashes every four canaries or so). UI Thread checks are added everywhere. The code around CrAVFoundationDeviceObserver::dealloc and -stopObserving is refactored in order to avoid a redundant search-for-device in |monitoredDevices_|. BUG=366087 Review URL: https://codereview.chromium.org/368613002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282723 0039d316-1c4b-4281-b951-d872f2087c98 --- content/browser/device_monitor_mac.mm | 209 ++++++++++++++++++++++------------ 1 file changed, 135 insertions(+), 74 deletions(-) diff --git a/content/browser/device_monitor_mac.mm b/content/browser/device_monitor_mac.mm index eb429afe6402..2d872d74775b 100644 --- a/content/browser/device_monitor_mac.mm +++ b/content/browser/device_monitor_mac.mm @@ -10,11 +10,14 @@ #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" +using content::BrowserThread; + namespace { // This class is used to keep track of system devices names and their types. @@ -216,57 +219,127 @@ 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 monitoredDevices_; + std::set > monitoredDevices_; } -- (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver; -- (void)startObserving:(CrAVCaptureDevice*)device; +- (id)initWithOnChangedCallback:(const base::Closure&)callback; +- (void)startObserving:(base::scoped_nsobject)device; - (void)stopObserving:(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 on this thread except for the expensive device enumerations which +// are run on Device Thread. class SuspendObserverDelegate : public base::RefCountedThreadSafe { public: - explicit SuspendObserverDelegate(DeviceMonitorMacImpl* monitor) - : avfoundation_monitor_impl_(monitor) { - device_thread_checker_.DetachFromThread(); - } - - void OnDeviceChanged(); - void StartObserver(); - void ResetDeviceMonitorOnUIThread(); + explicit SuspendObserverDelegate(DeviceMonitorMacImpl* monitor); + + // Create |suspend_observer_| for all devices and register OnDeviceChanged() + // as its change callback. Schedule bottom half in DoStartObserver(). + void StartObserver( + const scoped_refptr& device_thread); + // Enumerate devices in |device_thread| and run the bottom half in + // DoOnDeviceChange(). |suspend_observer_| calls back here on suspend event, + // and our parent AVFoundationMonitorImpl calls on connect/disconnect device. + void OnDeviceChanged( + const scoped_refptr& device_thread); + // Remove the device monitor's weak reference. Remove ourselves as suspend + // notification observer from |suspend_observer_|. + void ResetDeviceMonitor(); private: friend class base::RefCountedThreadSafe; - virtual ~SuspendObserverDelegate() {} + virtual ~SuspendObserverDelegate(); - void OnDeviceChangedOnUIThread( - const std::vector& snapshot_devices); + // Bottom half of StartObserver(), starts |suspend_observer_| for all devices. + // Assumes that |devices| has been retained prior to being called, and + // releases it internally. + void DoStartObserver(NSArray* devices); + // Bottom half of OnDeviceChanged(), starts |suspend_observer_| for current + // devices and composes a snapshot of them to send it to + // |avfoundation_monitor_impl_|. Assumes that |devices| has been retained + // prior to being called, and releases it internally. + void DoOnDeviceChanged(NSArray* devices); - base::ThreadChecker device_thread_checker_; base::scoped_nsobject 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& 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]); + + // Enumerate the devices in Device thread and post the observers start to be + // done on UI thread. The devices array is retained in |device_thread| and + // released in DoStartObserver(). + base::PostTaskAndReplyWithResult(device_thread, FROM_HERE, + base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }), + base::Bind(&SuspendObserverDelegate::DoStartObserver, this)); +} + +void SuspendObserverDelegate::OnDeviceChanged( + const scoped_refptr& device_thread) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + // Enumerate the devices in Device thread and post the consolidation of the + // new devices and the old ones to be done on UI thread. The devices array + // is retained in |device_thread| and released in DoOnDeviceChanged(). + PostTaskAndReplyWithResult(device_thread, FROM_HERE, + base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }), + 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) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + base::scoped_nsobject auto_release(devices); + for (CrAVCaptureDevice* device in devices) { + base::scoped_nsobject device_ptr([device retain]); + [suspend_observer_ startObserving:device_ptr]; + } +} + +void SuspendObserverDelegate::DoOnDeviceChanged(NSArray* devices) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + base::scoped_nsobject auto_release(devices); std::vector snapshot_devices; for (CrAVCaptureDevice* device in devices) { - [suspend_observer_ startObserving:device]; + base::scoped_nsobject 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 +355,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& 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 +366,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 +378,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 +394,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 +410,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::iterator it = monitoredDevices_.begin(); + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + std::set >::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)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 { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(device != nil); - std::set::iterator found = + + std::set >::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:(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(context)]; } -- 2.11.4.GIT