Fix race when setting Desktop window id.

content/browser/renderer_host/media/video_capture_manager.cc

Index: content/browser/renderer_host/media/video_capture_manager.cc
diff --git a/content/browser/renderer_host/media/video_capture_manager.cc b/content/browser/renderer_host/media/video_capture_manager.cc
index 26b3a701105330ccc00046b00f6848a880ba5e8e..3f6e0f3b1e1e8de70790d968fd4f61810aaf3326 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -113,6 +113,9 @@ VideoCaptureManager::DeviceEntry::DeviceEntry(
 
 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(
@@ -359,8 +362,15 @@ void VideoCaptureManager::OnDeviceStarted(
           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();
@@ -408,13 +418,6 @@ VideoCaptureManager::DoStartDeviceOnDeviceThread(
       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;
@@ -615,23 +618,32 @@ void VideoCaptureManager::SetDesktopCaptureWindowId(
   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;
   }
 
@@ -651,7 +663,7 @@ void VideoCaptureManager::SetDesktopCaptureWindowId(
       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(
@@ -849,15 +861,4 @@ void VideoCaptureManager::SetDesktopCaptureWindowIdOnDeviceThread(
 #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