Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
276573009:
DeviceMonitorMac AVFoundation: move mgmt of |suspend_observer_| to a class living in Device Thread. (Closed)
|
Created:
6 years, 7 months ago by mcasas Modified:
6 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDeviceMonitorMac AVFoundation: move mgmt of |suspend_observer_| to a class living in Device Thread.
This CL addresses a potential problem due to a race:
|suspend_observer_| [1] could have been deallocated in
~AVFoundationMonitorImpl while still being in use
in Device Thread [2]. This race didn't show up in normal
circumstances since it needed the Device Thread to
be active for a very short time; xians@ stumbled upon
it during some unrelated code activities that made a
browser_test fail.
This CL then cleans up the management of
|suspend_observer_| from the AVFoundationMonitorImpl
to a new class SuspendObserverDelegate, the former
living exclusively in UI thread, the latter in Device
Thread. SuspendObserverDelegate gets the Device
Thread parts of AVFoundationMonitorImpl.
The new class sits between AVFoundationMonitorImpl
and the Objective-C++ CrAVFoundationDeviceObserver.
[1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/device_monitor_mac.mm&sq=package:chromium&type=cs&l=259
[2] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/device_monitor_mac.mm&sq=package:chromium&type=cs&l=336
BUG=288562, 372418
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271438
Patch Set 1 : #
Total comments: 8
Patch Set 2 : tommi@s comments. #
Total comments: 2
Patch Set 3 : tommi@s thread-hopping proposal for OnDeviceChanged() back PostTask #
Total comments: 6
Patch Set 4 : tommi@s 2nd round of comments. #
Total comments: 12
Patch Set 5 : rsesek@ comments #Messages
Total messages: 21 (0 generated)
tommi@ PTAL rsesek@ FYI/PTAL.
https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... content/browser/device_monitor_mac.mm:256: DeviceMonitorMacImpl* avfoundation_monitor_impl_; I don't see where this variable is NULLed. It needs to be set to NULL when the DeviceMonitorMacImpl is deleted and that needs to happen in a thread safe way to make sure the pointer isn't being used on the device thread. https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... content/browser/device_monitor_mac.mm:281: base::MessageLoopForUI::current()->PostTask(FROM_HERE, base::MessageLoopForUI::current() doesn't return the UI thread loop. It returns the current message loop and DCHECKs that the type of the current loop is TYPE_UI. See here: https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... content/browser/device_monitor_mac.mm:296: // per device. Owns a SuspendObserverDelegate living in |deivce_task_runner_| device_task_runner_ https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... content/browser/device_monitor_mac.mm:348: DCHECK(thread_checker_.CalledOnValidThread()); here you would need to do something like: suspend_observer_delegate_->ResetDeviceMonitor(); to make sure the object on the device thread never pulls a use-after-free.
tommi@ PTAL. https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... content/browser/device_monitor_mac.mm:256: DeviceMonitorMacImpl* avfoundation_monitor_impl_; On 2014/05/13 14:43:41, tommi wrote: > I don't see where this variable is NULLed. It needs to be set to NULL when the > DeviceMonitorMacImpl is deleted and that needs to happen in a thread safe way to > make sure the pointer isn't being used on the device thread. Done. https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... content/browser/device_monitor_mac.mm:281: base::MessageLoopForUI::current()->PostTask(FROM_HERE, On 2014/05/13 14:43:41, tommi wrote: > base::MessageLoopForUI::current() doesn't return the UI thread loop. It returns > the current message loop and DCHECKs that the type of the current loop is > TYPE_UI. See here: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... You're totally right. I meant using: BrowserThread::PostTask(BrowserThread::UI, ...) https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... content/browser/device_monitor_mac.mm:296: // per device. Owns a SuspendObserverDelegate living in |deivce_task_runner_| On 2014/05/13 14:43:41, tommi wrote: > device_task_runner_ Done. https://codereview.chromium.org/276573009/diff/20001/content/browser/device_m... content/browser/device_monitor_mac.mm:348: DCHECK(thread_checker_.CalledOnValidThread()); On 2014/05/13 14:43:41, tommi wrote: > here you would need to do something like: > > suspend_observer_delegate_->ResetDeviceMonitor(); > > to make sure the object on the device thread never pulls a use-after-free. Done.
https://codereview.chromium.org/276573009/diff/80001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/276573009/diff/80001/content/browser/device_m... content/browser/device_monitor_mac.mm:293: base::Unretained(avfoundation_monitor_impl_), snapshot_devices)); This is unfortunately not safe either. The pointer can be non-NULL here but when the task runs, it can be NULL. Here's another proposal: Since SuspendObserverDelegate is reference counted, I propose you post a task to |this| (*not* using base::Unretained). In the function that gets called back on the UI thread, DCHECK(<ui thread>), check avfoundation_monitor_impl_ for being NULL and in the case of it not being NULL, call DeviceMonitorMacImpl::ConsolidateDevicesListAndNotify synchronously. Once that's done, you can remove the |lock_| member variable.
PTAL. https://codereview.chromium.org/276573009/diff/80001/content/browser/device_m... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/276573009/diff/80001/content/browser/device_m... content/browser/device_monitor_mac.mm:293: base::Unretained(avfoundation_monitor_impl_), snapshot_devices)); On 2014/05/15 13:35:39, tommi wrote: > This is unfortunately not safe either. The pointer can be non-NULL here but > when the task runs, it can be NULL. > > Here's another proposal: > > Since SuspendObserverDelegate is reference counted, I propose you post a task to > |this| (*not* using base::Unretained). > > In the function that gets called back on the UI thread, DCHECK(<ui thread>), > check avfoundation_monitor_impl_ for being NULL and in the case of it not being > NULL, call DeviceMonitorMacImpl::ConsolidateDevicesListAndNotify synchronously. > > Once that's done, you can remove the |lock_| member variable. Good suggestion. Done.
lgtm https://codereview.chromium.org/276573009/diff/140001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/276573009/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:14: #include "base/synchronization/lock.h" remove this now? https://codereview.chromium.org/276573009/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:259: base::ThreadChecker thread_checker_; nit: device_thread_checker_ https://codereview.chromium.org/276573009/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:311: snapshot_devices); indent
avi@ OWNERS stamp please. rsesek@ ping just in case. https://codereview.chromium.org/276573009/diff/140001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/276573009/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:14: #include "base/synchronization/lock.h" On 2014/05/15 15:43:22, tommi wrote: > remove this now? Done. https://codereview.chromium.org/276573009/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:259: base::ThreadChecker thread_checker_; On 2014/05/15 15:43:22, tommi wrote: > nit: device_thread_checker_ Done. https://codereview.chromium.org/276573009/diff/140001/content/browser/device_... content/browser/device_monitor_mac.mm:311: snapshot_devices); On 2014/05/15 15:43:22, tommi wrote: > indent Done.
https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:219: // manipulated by SuspendObserverDelegate in Device Thread. -stopObserving is Can you simplify this to say that "Created, manipulated, and destroyed on the Device Thread by SuspendedObserverDelegate"? https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:223: SuspendObserverDelegate* receiver_; nit: comment " // weak" at the end of the line https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:243: SuspendObserverDelegate(DeviceMonitorMacImpl* monitor) explicit https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:256: friend class base::RefCountedThreadSafe<SuspendObserverDelegate>; nit: friends come first in the private section https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:299: DCHECK(base::MessageLoopForUI::IsCurrent()); More common to do DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:305: DCHECK(base::MessageLoopForUI::IsCurrent()); Same.
rsesek@ PTAL. https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... File content/browser/device_monitor_mac.mm (right): https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:219: // manipulated by SuspendObserverDelegate in Device Thread. -stopObserving is On 2014/05/15 17:01:00, rsesek wrote: > Can you simplify this to say that "Created, manipulated, and destroyed on the > Device Thread by SuspendedObserverDelegate"? Done. https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:223: SuspendObserverDelegate* receiver_; On 2014/05/15 17:01:00, rsesek wrote: > nit: comment " // weak" at the end of the line Done. https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:243: SuspendObserverDelegate(DeviceMonitorMacImpl* monitor) On 2014/05/15 17:01:00, rsesek wrote: > explicit Done. https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:256: friend class base::RefCountedThreadSafe<SuspendObserverDelegate>; On 2014/05/15 17:01:00, rsesek wrote: > nit: friends come first in the private section Done. https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:299: DCHECK(base::MessageLoopForUI::IsCurrent()); On 2014/05/15 17:01:00, rsesek wrote: > More common to do > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); Done. https://codereview.chromium.org/276573009/diff/160001/content/browser/device_... content/browser/device_monitor_mac.mm:305: DCHECK(base::MessageLoopForUI::IsCurrent()); On 2014/05/15 17:01:00, rsesek wrote: > Same. Done.
lgtm
avi@ PTAL.
avi@ ping OWNERS stamp please.
lgtm stamp
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/276573009/180001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
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/276573009/180001
Message was sent while issue was closed.
Change committed as 271438 |
