VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on error

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 7cd548ab0f0eb1f194216c125eb63ec85ae58d05..d83a3bfb9e8b89e214cbb8f98ffabb31011447b9 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -659,8 +659,7 @@ void VideoCaptureManager::StartCaptureForClient(
     VideoCaptureControllerEventHandler* client_handler,
     const DoneCB& done_cb) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DVLOG(1) << "VideoCaptureManager::StartCaptureForClient #" << session_id
-           << ", request: "
+  DVLOG(1) << __func__ << ", session_id = " << session_id << ", request: "
            << media::VideoCaptureFormat::ToString(params.requested_format);
 
   DeviceEntry* entry = GetOrCreateDeviceEntry(session_id, params);
@@ -712,21 +711,22 @@ void VideoCaptureManager::StopCaptureForClient(
     }
   } else {
     LogVideoCaptureEvent(VIDEO_CAPTURE_STOP_CAPTURE_DUE_TO_ERROR);
-    SessionMap::iterator it;
-    for (it = sessions_.begin(); it != sessions_.end(); ++it) {
-      if (it->second.type == entry->stream_type &&
-          it->second.id == entry->id) {
-        listener_->Aborted(it->second.type, it->first);
+    for (auto it : sessions_) {
+      if (it.second.type == entry->stream_type && it.second.id == entry->id) {
+        listener_->Aborted(it.second.type, it.first);
+        // Aborted() call might synchronously destroy |entry|, recheck.

[xianglu, 2016/10/14 23:28:22]

If the entry is always going to be deleted after Aborted() on the same thread,
why do we need to get and check again? Can we safely return?

[mcasas, 2016/10/14 23:44:10]

The |entry| might still be alive if there's more than
one client using it (e.g. two tabs capturing using the
same webcam) -- that's why the method below is
called DestroyDeviceEntryIfNoClients(). Or that's 
my understanding, since this class and associated
fellows are very complex to reason about IMHO.

+        entry = GetDeviceEntryByController(controller);
+        if (!entry)
+          return;
         break;
       }
     }
   }
 
   // Detach client from controller.
-  media::VideoCaptureSessionId session_id =
+  const media::VideoCaptureSessionId session_id =
       controller->RemoveClient(client_id, client_handler);
-  DVLOG(1) << "VideoCaptureManager::StopCaptureForClient, session_id = "
-           << session_id;
+  DVLOG(1) << __func__ << ", session_id = " << session_id;
 
   // If controller has no more clients, delete controller and device.
   DestroyDeviceEntryIfNoClients(entry);