Fix race when setting Desktop window id.

content/browser/renderer_host/media/video_capture_manager.cc

 // 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/renderer_host/media/video_capture_manager.h"
 
 #include <algorithm>
 #include <set>
 
 #include "base/bind.h"
@@ skipping 95 matching lines @@
     MediaStreamType stream_type,
     const std::string& id,
     scoped_ptr<VideoCaptureController> controller)
     : serial_id(g_device_start_id++),
       stream_type(stream_type),
       id(id),
       video_capture_controller_(controller.Pass()) {}
 
 VideoCaptureManager::DeviceEntry::~DeviceEntry() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  // CHECK that the media::VideoCaptureDevice is not

[miu, 2015/02/06 21:51:47]

s/media::VideoCaptureDevice/media::VideoCaptureDevice::DeviceEntry/

[perkj_chrome, 2015/02/09 08:27:41]

I want to check that the entry does not own a media::VideoCaptureDevice when its
deleted. Refrased

+  // deleted on the wrong thread.
+  CHECK(video_capture_device_ == NULL);

[Sergey Ulanov, 2015/02/06 22:20:03]

s/NULL/nullptr/, or CHECK(!video_capture_device_)

[perkj_chrome, 2015/02/09 08:27:41]

ok. I am pretty sure the race is the problem that causes the crash. So I change
this to a DCHECK as well.

 }
 
 void VideoCaptureManager::DeviceEntry::SetVideoCaptureDevice(
     scoped_ptr<media::VideoCaptureDevice> device) {
   DCHECK(thread_checker_.CalledOnValidThread());
   video_capture_device_.swap(device);
 }
 
 scoped_ptr<media::VideoCaptureDevice>
 VideoCaptureManager::DeviceEntry::ReleaseVideoCaptureDevice() {
@@ skipping 226 matching lines @@
       // PostTask failed. The device must be stopped anyway.
       device_ptr->StopAndDeAllocate();
     }
   } else {
     DeviceEntries::iterator entry_it = std::find_if(
         devices_.begin(), devices_.end(),
         [serial_id] (const DeviceEntry* e) {
           return e->serial_id == serial_id;
         });
     DCHECK(entry_it != devices_.end());
-    DCHECK(!(*entry_it)->video_capture_device());
-    (*entry_it)->SetVideoCaptureDevice(device.Pass());
+    DeviceEntry* entry = *entry_it;
+    DCHECK(!entry->video_capture_device());
+    entry->SetVideoCaptureDevice(device.Pass());
+
+    if (entry->stream_type == MEDIA_DESKTOP_VIDEO_CAPTURE) {
+      const media::VideoCaptureSessionId session_id =
+          device_start_queue_.front().session_id();
+      MaybePostDesktopCaptureWindowId(session_id);
+    }
   }
 
   device_start_queue_.pop_front();
   HandleQueuedStartRequest();
 }
 
 scoped_ptr<media::VideoCaptureDevice>
 VideoCaptureManager::DoStartDeviceOnDeviceThread(
     media::VideoCaptureSessionId session_id,
     const std::string& id,
@@ skipping 27 matching lines @@
       DesktopMediaID desktop_id = DesktopMediaID::Parse(id);
 #if defined(USE_AURA)
       if (desktop_id.type == DesktopMediaID::TYPE_AURA_WINDOW) {
         video_capture_device.reset(
             DesktopCaptureDeviceAura::Create(desktop_id));
       } else
 #endif
       if (desktop_id.type != DesktopMediaID::TYPE_NONE &&
           desktop_id.type != DesktopMediaID::TYPE_AURA_WINDOW) {
         video_capture_device = DesktopCaptureDevice::Create(desktop_id);
-        if (notification_window_ids_.find(session_id) !=
-            notification_window_ids_.end()) {
-          static_cast<DesktopCaptureDevice*>(video_capture_device.get())
-              ->SetNotificationWindowId(notification_window_ids_[session_id]);
-          VLOG(2) << "Screen capture notification window passed for session "
-                  << session_id;
-        }
       }
 #endif  // defined(ENABLE_SCREEN_CAPTURE)
       break;
     }
     default: {
       NOTIMPLEMENTED();
       break;
     }
   }
 
@@ skipping 180 matching lines @@
   }
   return true;
 }
 
 void VideoCaptureManager::SetDesktopCaptureWindowId(
     media::VideoCaptureSessionId session_id,
     gfx::NativeViewId window_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   VLOG(2) << "SetDesktopCaptureWindowId called for session " << session_id;
 
+  notification_window_ids_[session_id] = window_id;
+  MaybePostDesktopCaptureWindowId(session_id);
+}
+
+void VideoCaptureManager::MaybePostDesktopCaptureWindowId(
+    media::VideoCaptureSessionId session_id) {
   SessionMap::iterator session_it = sessions_.find(session_id);
   if (session_it == sessions_.end()) {
-    VLOG(2) << "Session not found, will save the notification window.";
-    device_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(
-            &VideoCaptureManager::SaveDesktopCaptureWindowIdOnDeviceThread,
-            this,
-            session_id,
-            window_id));
+    return;
+  }
+
+  if (notification_window_ids_.find(session_id) ==
+      notification_window_ids_.end()) {
+    DVLOG(2) << "Notification window id not set for screen capture.";
     return;
   }
 
   DeviceEntry* const existing_device =
       GetDeviceEntryForMediaStreamDevice(session_it->second);
   if (!existing_device) {
-    VLOG(2) << "Failed to find an existing device.";
+    DVLOG(2) << "Failed to find an existing screen capture device.";
     return;
   }
 
+  if (!existing_device->video_capture_device()) {
+    DVLOG(2) << "Screen capture device not yet started.";
+    return;
+  }
+
   DCHECK_EQ(MEDIA_DESKTOP_VIDEO_CAPTURE, existing_device->stream_type);
   DesktopMediaID id = DesktopMediaID::Parse(existing_device->id);
   if (id.type == DesktopMediaID::TYPE_NONE ||
       id.type == DesktopMediaID::TYPE_AURA_WINDOW) {
     VLOG(2) << "Video capture device type mismatch.";
     return;
   }
 
   // Post |existing_device->video_capture_device| to the VideoCaptureDevice to
   // the device_task_runner_. This is safe since the device is destroyed on the
   // device_task_runner_.
   device_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&VideoCaptureManager::SetDesktopCaptureWindowIdOnDeviceThread,
                  this,
                  existing_device->video_capture_device(),
-                 window_id));
+                 notification_window_ids_[session_id]));

[Sergey Ulanov, 2015/02/06 22:20:03]

I think you also want to remove the value from notification_window_ids_.
Otherwise it will grow indefinitely.

[perkj_chrome, 2015/02/09 08:27:41]

ok, so this will fix an old bug as well? It looks like that has been the case
for some time.

 }
 
 void VideoCaptureManager::DoStopDeviceOnDeviceThread(
     scoped_ptr<media::VideoCaptureDevice> device) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StopDeviceTime");
   DCHECK(IsOnDeviceThread());
   device->StopAndDeAllocate();
   DVLOG(3) << "DoStopDeviceOnDeviceThread";
 }
 
@@ skipping 177 matching lines @@
     gfx::NativeViewId window_id) {
   DCHECK(IsOnDeviceThread());
 #if defined(ENABLE_SCREEN_CAPTURE)
   DesktopCaptureDevice* desktop_device =
       static_cast<DesktopCaptureDevice*>(device);
   desktop_device->SetNotificationWindowId(window_id);
   VLOG(2) << "Screen capture notification window passed on device thread.";
 #endif
 }
 
-void VideoCaptureManager::SaveDesktopCaptureWindowIdOnDeviceThread(
-    media::VideoCaptureSessionId session_id,
-    gfx::NativeViewId window_id) {
-  DCHECK(IsOnDeviceThread());
-  DCHECK(notification_window_ids_.find(session_id) ==
-         notification_window_ids_.end());
-  notification_window_ids_[session_id] = window_id;
-  VLOG(2) << "Screen capture notification window saved for session "
-          << session_id << " on device thread.";
-}
-
 }  // namespace content