Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
368613002:
DeviceMonitorMac: move CrAVFoundationDeviceObserver and most of SuspendObserverDelegate to UI Thread (Closed)
|
Created:
6 years, 5 months ago by mcasas Modified:
6 years, 5 months ago Reviewers:
Robert Sesek, tommi (sloooow) - chröme, Avi (use Gerrit), jochen (gone - plz use gerrit), piman CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionDeviceMonitorMac: 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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282723
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Suspended/Resumed Event posts the notification to Device Thread. #
Total comments: 10
Patch Set 3 : tommi@s comments #
Total comments: 6
Patch Set 4 : tommi@s 2nd round of comments #
Total comments: 15
Patch Set 5 : rsesek@ comments, adding reference counting to |device| manipulation, simplified use of Device Thre… #
Total comments: 20
Patch Set 6 : rsesek@ and tommi@ comments #
Total comments: 8
Patch Set 7 : Comments and using scoped_nsobject<> for auto releasing |devices| #
Total comments: 4
Patch Set 8 : jochen@ comments #Messages
Total messages: 30 (0 generated)
rsesek@, tommi@ PTAL.
I'm holding off with the review as per our offline discussion - rsesek, you might as well wait too :)
Should be cleaned up now, and beefed up the explanation. PTAL.
I'm finding it really hard to follow the threading logic of this file—it really seems too complicated. Maybe fresh eyes tomorrow will help. Tommi: WDYT? https://codereview.chromium.org/368613002/diff/140001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:301: base::BindBlock(^{ nit: indent 2 more spaces https://codereview.chromium.org/368613002/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:302: suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc] This seems like questionably unsafe data access. https://codereview.chromium.org/368613002/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:303: initWithChangeReceiver:this]); })); Closing brace for the block should go on the next line if it doesn't all fit on one line.
Yes same situation here. Miguel went over this together yesterday and there are quite a few places where we've been touching variables on >1 threads without synchronization. So Miguel is adding DCHECKs liberally to enforce that the right thing is done and to also make the code easer to understand. The threading model for the observer should basically be "UI thread only" before it was mixed between UI and device, but there wasn't any benefit for doing some operations on the device thread only to have some (more frequent) operations happen on the UI thread anyway (the actual notifications). This mix was also causing confusion and bugs. So, now the notifications + [un]registration happens on the UI thread. The lifetime of the observer is also strictly tied to the UI thread. This is also better from the API pov since QTKit has problems with other threads and although AVFoundation does support other threads, it delivers notifications on the UI thread. So, what stays on the device thread, is device enumeration. Miguel - can you verify/correct the above understanding? On Wed, Jul 2, 2014 at 9:44 PM, <rsesek@chromium.org> wrote: > I'm finding it really hard to follow the threading logic of this file--it > really > seems too complicated. Maybe fresh eyes tomorrow will help. Tommi: WDYT? > > > https://codereview.chromium.org/368613002/diff/140001/ > content/browser/device_monitor_mac.mm > File content/browser/device_monitor_mac.mm (right): > > https://codereview.chromium.org/368613002/diff/140001/ > content/browser/device_monitor_mac.mm#newcode301 > content/browser/device_monitor_mac.mm:301: base::BindBlock(^{ > nit: indent 2 more spaces > > https://codereview.chromium.org/368613002/diff/140001/ > content/browser/device_monitor_mac.mm#newcode302 > content/browser/device_monitor_mac.mm:302: > suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc] > This seems like questionably unsafe data access. > > https://codereview.chromium.org/368613002/diff/140001/ > content/browser/device_monitor_mac.mm#newcode303 > content/browser/device_monitor_mac.mm:303: > initWithChangeReceiver:this]); })); > Closing brace for the block should go on the next line if it doesn't all > fit on one line. > > https://codereview.chromium.org/368613002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Correct understanding, everything on UI thread except operations involving device enumerations, namely SuspendObserverDelegate::StartObserver() and SuspendObserverDelegate::OnDeviceChanged(). I will add some more comments. On 2014/07/03 10:17:00, tommi wrote: > Yes same situation here. Miguel went over this together yesterday and > there are quite a few places where we've been touching variables on >1 > threads without synchronization. So Miguel is adding DCHECKs liberally to > enforce that the right thing is done and to also make the code easer to > understand. > > The threading model for the observer should basically be "UI thread only" > before it was mixed between UI and device, but there wasn't any benefit > for doing some operations on the device thread only to have some (more > frequent) operations happen on the UI thread anyway (the actual > notifications). This mix was also causing confusion and bugs. > > So, now the notifications + [un]registration happens on the UI thread. The > lifetime of the observer is also strictly tied to the UI thread. This is > also better from the API pov since QTKit has problems with other threads > and although AVFoundation does support other threads, it delivers > notifications on the UI thread. > > So, what stays on the device thread, is device enumeration. > > Miguel - can you verify/correct the above understanding? > > > On Wed, Jul 2, 2014 at 9:44 PM, <mailto:rsesek@chromium.org> wrote: > > > I'm finding it really hard to follow the threading logic of this file--it > > really > > seems too complicated. Maybe fresh eyes tomorrow will help. Tommi: WDYT? > > > > > > https://codereview.chromium.org/368613002/diff/140001/ > > content/browser/device_monitor_mac.mm > > File content/browser/device_monitor_mac.mm (right): > > > > https://codereview.chromium.org/368613002/diff/140001/ > > content/browser/device_monitor_mac.mm#newcode301 > > content/browser/device_monitor_mac.mm:301: base::BindBlock(^{ > > nit: indent 2 more spaces > > > > https://codereview.chromium.org/368613002/diff/140001/ > > content/browser/device_monitor_mac.mm#newcode302 > > content/browser/device_monitor_mac.mm:302: > > suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc] > > This seems like questionably unsafe data access. > > > > https://codereview.chromium.org/368613002/diff/140001/ > > content/browser/device_monitor_mac.mm#newcode303 > > content/browser/device_monitor_mac.mm:303: > > initWithChangeReceiver:this]); })); > > Closing brace for the block should go on the next line if it doesn't all > > fit on one line. > > > > https://codereview.chromium.org/368613002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Added posting suspended/resume events to Device Thread, apologies for forgetting it before -- lucky the thread checks. PTAL. https://codereview.chromium.org/368613002/diff/140001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:301: base::BindBlock(^{ On 2014/07/02 19:44:45, rsesek wrote: > nit: indent 2 more spaces Done. https://codereview.chromium.org/368613002/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:302: suspend_observer_.reset([[CrAVFoundationDeviceObserver alloc] On 2014/07/02 19:44:45, rsesek wrote: > This seems like questionably unsafe data access. I'm not sure I understand. Isn't it the same as what was done before, carried out in UI thread? https://codereview.chromium.org/368613002/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:303: initWithChangeReceiver:this]); })); On 2014/07/02 19:44:45, rsesek wrote: > Closing brace for the block should go on the next line if it doesn't all fit on > one line. Done.
https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:224: scoped_refptr<SuspendObserverDelegate> receiver_; 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. https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:277: base::BindBlock(^{ [suspend_observer_ startObserving:device]; })); 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. https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:307: deviceMessageLoop:device_message_loop]); Is passing the device_message_loop this way to the bound block, another way of just doing PostTaskAndReply? (or is there more to it?) https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:311: base::BindBlock(^{ [suspend_observer_ startObserving:device]; })); 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?
tommi@ PTAL. https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:224: scoped_refptr<SuspendObserverDelegate> receiver_; On 2014/07/03 13:30:17, tommi wrote: > 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. That makes sense. Will do. https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:277: base::BindBlock(^{ [suspend_observer_ startObserving:device]; })); On 2014/07/03 13:30:17, tommi wrote: > 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. |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...? https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:307: deviceMessageLoop:device_message_loop]); On 2014/07/03 13:30:17, tommi wrote: > Is passing the device_message_loop this way to the bound block, another way of > just doing PostTaskAndReply? (or is there more to it?) |device_message_loop| is cached inside CrAVFoundationDeviceObserver and used repeatedly (unless changed to callback like you propose earlier). https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:311: base::BindBlock(^{ [suspend_observer_ startObserving:device]; })); On 2014/07/03 13:30:17, tommi wrote: > 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? l.309 ought to be executed in Device Thread, is the time consuming device enumeration.
https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:277: base::BindBlock(^{ [suspend_observer_ startObserving:device]; })); On 2014/07/04 14:08:58, mcasas wrote: > On 2014/07/03 13:30:17, tommi wrote: > > 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. > > |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...? 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. https://codereview.chromium.org/368613002/diff/180001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/180001/content/browser/device_... content/browser/device_monitor_mac.mm:225: SuspendObserverDelegate* receiver_; // Weak. you don't need this variable anymore https://codereview.chromium.org/368613002/diff/180001/content/browser/device_... content/browser/device_monitor_mac.mm:230: - (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver Since this method doesn't need the receiver pointer anymore, its name should probably be changed (e.g. to simply "init" or better yet, "initialize") https://codereview.chromium.org/368613002/diff/180001/content/browser/device_... content/browser/device_monitor_mac.mm:303: base::Closure on_device_changed_callback = media::BindToCurrentLoop( is media::BindToCurrentLoop used elsewhere in content? It feels like this is a utility method for media so using it in content might not be a good thing... I wonder if something comparable is available in base?
Thanks for the explanation. That plus fresh eyes makes this more clear. I'm definitely in favor of adding more threading assertions throughout this code. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Unless QTKit is going away soon, I think it may make sense to separate out the two backends from this file. It would probably make it easier to follow. (In another CL). https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:52: class CrAVCaptureDeviceRefCounted : I don't understand the point of this class. CrAVCaptureDevice is already thread-safely refcounted by being a NSObject. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:236: base::Closure onDeviceChangedCallback_; nit: blank line after, and a comment (e.g. on what thread is this run?) https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:257: : avfoundation_monitor_impl_(monitor) { Please move the ctor and dtor out of line, like the rest of the methods. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:270: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); You could add a |using content::BrowserThread| if you wanted. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:317: base::BindBlock(^{ suspend_observer_.reset( nit: keep ^{ on this line but start the content of the block on the next https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:481: onDeviceChangedCallback_.Run(); Won't this run the callback on the UI thread, rather than the device thread?
PTAL. Updated description, bottom line: All operations in UI thread except the device enumerations, thanks to tommi@ wise guidance. This greatly simplifies the thread modelling. https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:277: base::BindBlock(^{ [suspend_observer_ startObserving:device]; })); On 2014/07/07 07:41:07, tommi wrote: > On 2014/07/04 14:08:58, mcasas wrote: > > On 2014/07/03 13:30:17, tommi wrote: > > > 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. > > > > |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...? > > 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. 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. https://codereview.chromium.org/368613002/diff/180001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/180001/content/browser/device_... content/browser/device_monitor_mac.mm:225: SuspendObserverDelegate* receiver_; // Weak. On 2014/07/07 07:41:07, tommi wrote: > you don't need this variable anymore Done. https://codereview.chromium.org/368613002/diff/180001/content/browser/device_... content/browser/device_monitor_mac.mm:230: - (id)initWithChangeReceiver:(SuspendObserverDelegate*)receiver On 2014/07/07 07:41:07, tommi wrote: > Since this method doesn't need the receiver pointer anymore, its name should > probably be changed (e.g. to simply "init" or better yet, "initialize") Changed, but still kept the initWithX as is part of Cocoa coding guidelines, "Add new keywords to the end of an existing method when you create a method that is more specific than the inherited one."... https://codereview.chromium.org/368613002/diff/180001/content/browser/device_... content/browser/device_monitor_mac.mm:303: base::Closure on_device_changed_callback = media::BindToCurrentLoop( On 2014/07/07 07:41:07, tommi wrote: > is media::BindToCurrentLoop used elsewhere in content? > It feels like this is a utility method for media so using it in content might > not be a good thing... I wonder if something comparable is available in base? Yes, f.i. in browser/renderer_host/media/video_capture_manager.cc and in other 29 locations not including unittests. Is a very useful class, I wonder if it should be ported? https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/07/07 22:47:50, rsesek wrote: > Unless QTKit is going away soon, I think it may make sense to separate out the > two backends from this file. It would probably make it easier to follow. (In > another CL). Total +1 !! http://crbug.com/392076 https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:52: class CrAVCaptureDeviceRefCounted : On 2014/07/07 22:47:50, rsesek wrote: > I don't understand the point of this class. CrAVCaptureDevice is already > thread-safely refcounted by being a NSObject. Gone; you're right it wasn't needed. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:236: base::Closure onDeviceChangedCallback_; On 2014/07/07 22:47:49, rsesek wrote: > nit: blank line after, and a comment (e.g. on what thread is this run?) Done. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:257: : avfoundation_monitor_impl_(monitor) { On 2014/07/07 22:47:49, rsesek wrote: > Please move the ctor and dtor out of line, like the rest of the methods. Done. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:270: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2014/07/07 22:47:49, rsesek wrote: > You could add a |using content::BrowserThread| if you wanted. Great, less verbosity, done. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:317: base::BindBlock(^{ suspend_observer_.reset( On 2014/07/07 22:47:50, rsesek wrote: > nit: keep ^{ on this line but start the content of the block on the next Done. https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:481: onDeviceChangedCallback_.Run(); On 2014/07/07 22:47:50, rsesek wrote: > Won't this run the callback on the UI thread, rather than the device thread? l.314 media::BindToCurrentLoop does the magic of caching the original message loop proxy and trampolining the PostTask back to it.
https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/200001/content/browser/device_... content/browser/device_monitor_mac.mm:481: onDeviceChangedCallback_.Run(); On 2014/07/08 15:11:46, mcasas wrote: > On 2014/07/07 22:47:50, rsesek wrote: > > Won't this run the callback on the UI thread, rather than the device thread? > > l.314 media::BindToCurrentLoop does the magic of caching the > original message loop proxy and trampolining the PostTask back > to it. Looks like this got dropped in the latest patch set? https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:251: void StartObserver( Comment these please. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:262: void DoStartObserver(NSArray* devices); Comment about ownership of |devices| here, after you fix the leaks. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:285: base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }), nit: indent 2 more spaces, same with the next line https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:309: void SuspendObserverDelegate::DoStartObserver(NSArray* devices) { |devices| is going to be leaked from line 284 / 295 https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:312: base::scoped_nsobject<CrAVCaptureDevice> device_ptr([device retain]); Do you need this line? The array owns the object. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:451: - (void)stopObserving:(const CrAVCaptureDevice*)device { ObjC objects are never const.
we're getting there :) https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:262: void DoStartObserver(NSArray* devices); On 2014/07/08 17:05:40, rsesek wrote: > Comment about ownership of |devices| here, after you fix the leaks. Does scoped_nsobject<> support this sort of scenario? i.e. could this be: void DoStartObserver(const scoped_nsobject<NSArray>& devices); https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:285: base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }), use scoped_nsobject wherever possible to reduce the likelihood of leaks like this. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:295: base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }), same here
https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:262: void DoStartObserver(NSArray* devices); On 2014/07/08 17:23:42, tommi wrote: > On 2014/07/08 17:05:40, rsesek wrote: > > Comment about ownership of |devices| here, after you fix the leaks. > > Does scoped_nsobject<> support this sort of scenario? i.e. could this be: > > void DoStartObserver(const scoped_nsobject<NSArray>& devices); 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.
https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:262: void DoStartObserver(NSArray* devices); On 2014/07/08 17:25:25, rsesek wrote: > On 2014/07/08 17:23:42, tommi wrote: > > On 2014/07/08 17:05:40, rsesek wrote: > > > Comment about ownership of |devices| here, after you fix the leaks. > > > > Does scoped_nsobject<> support this sort of scenario? i.e. could this be: > > > > void DoStartObserver(const scoped_nsobject<NSArray>& devices); > > 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. 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.
PTAL. I'd like to point out that CrAVFoundationDeviceObserver -startObserver: has scoped_nsobject<bla> as parameter. I'm not 100% sure if this does not fall straight into rsesek@ comments on the idiom of passing scoped_nsobject<T> as parameters. Alternatively, I can move the CrAVFoundationDeviceObserver to rely on NSObject explicit reference counting (i.e. via retain-release cycles). https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:251: void StartObserver( On 2014/07/08 17:05:40, rsesek wrote: > Comment these please. Done. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:262: void DoStartObserver(NSArray* devices); On 2014/07/08 17:29:29, tommi wrote: > On 2014/07/08 17:25:25, rsesek wrote: > > On 2014/07/08 17:23:42, tommi wrote: > > > On 2014/07/08 17:05:40, rsesek wrote: > > > > Comment about ownership of |devices| here, after you fix the leaks. > > > > > > Does scoped_nsobject<> support this sort of scenario? i.e. could this be: > > > > > > void DoStartObserver(const scoped_nsobject<NSArray>& devices); > > > > 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. > > 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. 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). https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:285: base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }), On 2014/07/08 17:05:40, rsesek wrote: > nit: indent 2 more spaces, same with the next line Done. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:285: base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }), On 2014/07/08 17:23:42, tommi wrote: > use scoped_nsobject wherever possible to reduce the likelihood of leaks like > this. As per the discussion above, I'll avoid using scoped_nsobject<T> in params, but add some info and the -release call. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:295: base::BindBlock(^{ return [[AVCaptureDeviceGlue devices] retain]; }), On 2014/07/08 17:23:42, tommi wrote: > same here Ditto: No scoped_nsobject<T> in params. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:309: void SuspendObserverDelegate::DoStartObserver(NSArray* devices) { On 2014/07/08 17:05:40, rsesek wrote: > |devices| is going to be leaked from line 284 / 295 Done here and inside DoOnDeviceChanged(), that also owns the |devices| array passed as parameter. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:312: base::scoped_nsobject<CrAVCaptureDevice> device_ptr([device retain]); On 2014/07/08 17:05:40, rsesek wrote: > Do you need this line? The array owns the object. We/I need to keep an extra reference for the scoped ptr to release it later on. https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:451: - (void)stopObserving:(const CrAVCaptureDevice*)device { On 2014/07/08 17:05:40, rsesek wrote: > ObjC objects are never const. Done.
https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/300001/content/browser/device_... content/browser/device_monitor_mac.mm:262: void DoStartObserver(NSArray* devices); On 2014/07/09 08:27:33, mcasas wrote: > On 2014/07/08 17:29:29, tommi wrote: > > On 2014/07/08 17:25:25, rsesek wrote: > > > On 2014/07/08 17:23:42, tommi wrote: > > > > On 2014/07/08 17:05:40, rsesek wrote: > > > > > Comment about ownership of |devices| here, after you fix the leaks. > > > > > > > > Does scoped_nsobject<> support this sort of scenario? i.e. could this be: > > > > > > > > void DoStartObserver(const scoped_nsobject<NSArray>& devices); > > > > > > 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. > > > > 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. > > 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). Since DoStartObserver releases the reference of |devices|, I think it's worth documenting that explicitly for this method. https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... content/browser/device_monitor_mac.mm:270: void DoOnDeviceChanged(NSArray* devices); same for DoOnDeviceChanged https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... content/browser/device_monitor_mac.mm:316: // DoStartObserver explicitly owns the devices array. nit: instead of "explicitly owns the devices array", can we say "DoStartObserver assumes that |devices| has been retained prior to being called and releases that reference internally". "explicitly owns" doesn't sound right to me for some reason (besides, the ownership is shared) https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... content/browser/device_monitor_mac.mm:323: [devices release]; another way to do this would be to create a scoped_nsobject instance at the top of the function: scoped_nsobject<NSArray> auto_release(devices); https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... content/browser/device_monitor_mac.mm:356: [devices release]; I would prefer the scoped_nsobject<> approach to releasing here at the bottom, for future-proofness.
tommi@ PTAL. https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... content/browser/device_monitor_mac.mm:270: void DoOnDeviceChanged(NSArray* devices); On 2014/07/09 09:19:39, tommi wrote: > same for DoOnDeviceChanged Done. https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... content/browser/device_monitor_mac.mm:316: // DoStartObserver explicitly owns the devices array. On 2014/07/09 09:19:40, tommi wrote: > nit: instead of "explicitly owns the devices array", can we say "DoStartObserver > assumes that |devices| has been retained prior to being called and releases that > reference internally". "explicitly owns" doesn't sound right to me for some > reason (besides, the ownership is shared) Done. https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... content/browser/device_monitor_mac.mm:323: [devices release]; On 2014/07/09 09:19:40, tommi wrote: > another way to do this would be to create a scoped_nsobject instance at the top > of the function: > > scoped_nsobject<NSArray> auto_release(devices); Done. https://codereview.chromium.org/368613002/diff/320001/content/browser/device_... content/browser/device_monitor_mac.mm:356: [devices release]; On 2014/07/09 09:19:40, tommi wrote: > I would prefer the scoped_nsobject<> approach to releasing here at the bottom, > for future-proofness. Done.
lgtm
sky@ RS please. (Usually I send Mac code to avi@ but he seems to be off).
I'm not a good reviewer for this code. Is there another owner you can fine?
piman@ RS please.
https://codereview.chromium.org/368613002/diff/380001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/380001/content/browser/device_... content/browser/device_monitor_mac.mm:17: #include "media/base/bind_to_current_loop.h" should not be needed? https://codereview.chromium.org/368613002/diff/380001/content/browser/device_... content/browser/device_monitor_mac.mm:20: using content::BrowserThread; no using content inside content. the entire file should be in namespace content
jochen@: Thanks, passing on to avi@ now. avi@ OWNERS RS / PTAL. https://codereview.chromium.org/368613002/diff/380001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/368613002/diff/380001/content/browser/device_... content/browser/device_monitor_mac.mm:17: #include "media/base/bind_to_current_loop.h" On 2014/07/11 12:55:22, jochen wrote: > should not be needed? Done. https://codereview.chromium.org/368613002/diff/380001/content/browser/device_... content/browser/device_monitor_mac.mm:20: using content::BrowserThread; On 2014/07/11 12:55:22, jochen wrote: > no using content inside content. the entire file should be in namespace content Objective-C++ classes cannot go inside a namespace (because Obj-C++ lives in a single namespace).
lgtm
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mcasas@chromium.org/368613002/400001
Message was sent while issue was closed.
Change committed as 282723
Message was sent while issue was closed.
LGTM |
