Avoid binding ref counted objects in ScreenObserverDelegate constructor

This CL adds a static constructor to ScreenObserverDelegate to avoid
a racy ref count operation.

As ScreenObserverDelegate is a ref counted class, base::Bind implicitly
increments the reference count, and releases it on the callback instance
destruction. If PostTask in the SOD ctor fails, or the posted task ran
soon before the ctor has completed, the ScreenObserverDelegate instance
may be destroyed immediately, and `new ScreenObserverDelegate` may
returns a stale pointer.

Bug: 866456
Change-Id: I7c64915529786b2f0738731c323aaf2f293dbc20
Reviewed-on: https://chromium-review.googlesource.com/1147896Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577624}

media/capture/video/chromeos/display_rotation_observer.cc

@@ -11,17 +11,25 @@
 
 namespace media {
 
+// static
+scoped_refptr<ScreenObserverDelegate> ScreenObserverDelegate::Create(
+    DisplayRotationObserver* observer,
+    scoped_refptr<base::SingleThreadTaskRunner> display_task_runner) {
+  auto delegate = base::WrapRefCounted(
+      new ScreenObserverDelegate(observer, display_task_runner));
+  display_task_runner->PostTask(
+      FROM_HERE,
+      base::BindOnce(&ScreenObserverDelegate::AddObserverOnDisplayThread,
+                     delegate));
+  return delegate;
+}
+
 ScreenObserverDelegate::ScreenObserverDelegate(
     DisplayRotationObserver* observer,
     scoped_refptr<base::SingleThreadTaskRunner> display_task_runner)
     : observer_(observer),
       display_task_runner_(std::move(display_task_runner)),
-      delegate_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
-  display_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&ScreenObserverDelegate::AddObserverOnDisplayThread,
-                     this));
-}
+      delegate_task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
 
 void ScreenObserverDelegate::RemoveObserver() {
   DCHECK(delegate_task_runner_->BelongsToCurrentThread());

media/capture/video/chromeos/display_rotation_observer.h

@@ -25,7 +25,7 @@ class ScreenObserverDelegate
     : public display::DisplayObserver,
       public base::RefCountedThreadSafe<ScreenObserverDelegate> {
  public:
-  ScreenObserverDelegate(
+  static scoped_refptr<ScreenObserverDelegate> Create(
       DisplayRotationObserver* observer,
       scoped_refptr<base::SingleThreadTaskRunner> display_task_runner);
 
@@ -36,6 +36,9 @@ class ScreenObserverDelegate
  private:
   friend class base::RefCountedThreadSafe<ScreenObserverDelegate>;
 
+  ScreenObserverDelegate(
+      DisplayRotationObserver* observer,
+      scoped_refptr<base::SingleThreadTaskRunner> display_task_runner);
   ~ScreenObserverDelegate() override;
 
   // DisplayObserver implementations.

media/capture/video/chromeos/video_capture_device_chromeos_halv3.cc

@@ -32,7 +32,7 @@ VideoCaptureDeviceChromeOSHalv3::VideoCaptureDeviceChromeOSHalv3(
       capture_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       camera_device_ipc_thread_(std::string("CameraDeviceIpcThread") +
                                 device_descriptor.device_id),
-      screen_observer_delegate_(new ScreenObserverDelegate(
+      screen_observer_delegate_(ScreenObserverDelegate::Create(
           this,
           std::move(task_runner_for_screen_observer))),
       lens_facing_(device_descriptor.facing),

media/capture/video/linux/video_capture_device_chromeos.cc

@@ -25,7 +25,7 @@ VideoCaptureDeviceChromeOS::VideoCaptureDeviceChromeOS(
     : VideoCaptureDeviceLinux(std::move(v4l2), device_descriptor),
       camera_config_(camera_config),
       screen_observer_delegate_(
-          new ScreenObserverDelegate(this, ui_task_runner)) {}
+          ScreenObserverDelegate::Create(this, ui_task_runner)) {}
 
 VideoCaptureDeviceChromeOS::~VideoCaptureDeviceChromeOS() {
   screen_observer_delegate_->RemoveObserver();