Resolve the problems where we can leak the system tray UI

content/browser/renderer_host/media/media_stream_manager.cc

Index: content/browser/renderer_host/media/media_stream_manager.cc
diff --git a/content/browser/renderer_host/media/media_stream_manager.cc b/content/browser/renderer_host/media/media_stream_manager.cc
index 1329b97d8c7b1d28da84667c995ae40956eac700..0549fde7097a778cbe3d307410cfc9ae37be687c 100644
--- a/content/browser/renderer_host/media/media_stream_manager.cc
+++ b/content/browser/renderer_host/media/media_stream_manager.cc
@@ -178,37 +178,6 @@ void MediaStreamManager::GenerateStream(MediaStreamRequester* requester,
   StartEnumeration(&new_request, label);
 }
 
-void MediaStreamManager::CancelRequests(MediaStreamRequester* requester) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DeviceRequests::iterator it = requests_.begin();
-  while (it != requests_.end()) {
-    if (it->second.requester == requester && !RequestDone(it->second)) {
-      // The request isn't complete, but there might be some devices already
-      // opened -> close them.
-      DeviceRequest* request = &(it->second);
-      if (request->state[content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE] ==
-          DeviceRequest::kOpening) {
-        for (StreamDeviceInfoArray::iterator it =
-             request->audio_devices.begin(); it != request->audio_devices.end();
-             ++it) {
-          audio_input_device_manager()->Close(it->session_id);
-        }
-      }
-      if (request->state[content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE] ==
-          DeviceRequest::kOpening) {
-        for (StreamDeviceInfoArray::iterator it =
-             request->video_devices.begin(); it != request->video_devices.end();
-             ++it) {
-          video_capture_manager()->Close(it->session_id);
-        }
-      }
-      requests_.erase(it++);
-    } else {
-      ++it;
-    }
-  }
-}
-
 void MediaStreamManager::CancelGenerateStream(const std::string& label) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
@@ -256,7 +225,9 @@ void MediaStreamManager::StopGeneratedStream(const std::string& label) {
          video_it != it->second.video_devices.end(); ++video_it) {
       video_capture_manager()->Close(video_it->session_id);
     }
-    if (it->second.type == DeviceRequest::kGenerateStream) {
+
+    if (it->second.type == DeviceRequest::kGenerateStream &&
+        RequestDone(it->second)) {
       NotifyObserverDevicesClosed(&(it->second));
     }
     requests_.erase(it);
@@ -350,13 +321,11 @@ void MediaStreamManager::StartEnumeration(
   // Need to make an asynchronous call to make sure the |requester| gets the
   // |label| before it would receive any event.
   if (new_request->type == DeviceRequest::kGenerateStream) {
-    BrowserThread::PostTask(BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&MediaStreamDeviceSettings::RequestCaptureDeviceUsage,
-                   base::Unretained(device_settings_.get()),
-                   request_label, new_request->render_process_id,
-                   new_request->render_view_id, new_request->options,
-                   new_request->security_origin));
+    device_settings_->RequestCaptureDeviceUsage(request_label,
+                                                new_request->render_process_id,
+                                                new_request->render_view_id,
+                                                new_request->options,
+                                                new_request->security_origin);
   }
 
   (*label) = request_label;
@@ -494,11 +463,7 @@ void MediaStreamManager::DevicesEnumerated(
         }
         break;
       default:
-        BrowserThread::PostTask(BrowserThread::IO,
-            FROM_HERE,
-            base::Bind(&MediaStreamDeviceSettings::AvailableDevices,
-                       base::Unretained(device_settings_.get()),
-                       *it, stream_type, devices));
+        device_settings_->AvailableDevices(*it, stream_type, devices);
     }
   }
   label_list.clear();
@@ -537,17 +502,21 @@ void MediaStreamManager::Error(MediaStreamType stream_type,
             it->second.requester->VideoDeviceFailed(it->first, device_idx);
           }
           GetDeviceManager(stream_type)->Close(capture_session_id);
-          devices->erase(device_it);
-        } else if (it->second.audio_devices.size()
-            + it->second.video_devices.size() <= 1) {
-          // 2. Device not opened and no other devices for this request ->
-          //    signal stream error and remove the request.
-          it->second.requester->StreamGenerationFailed(it->first);
-          requests_.erase(it);
+          // We don't erase the |requests_| here so that we can update the UI

[perkj_chrome, 2012/08/09 07:31:14]

? devices or requests? This previousely did devices->erase(device_it)

[no longer working on chromium, 2012/08/09 15:54:54]

Done.

+          // properly in StopGeneratedStream().
+          it->second.state[stream_type] = DeviceRequest::kError;
         } else {

[mflodman_chromium_OOO, 2012/08/09 09:57:05]

A comment regarding the request is not done?

[no longer working on chromium, 2012/08/09 15:54:54]

Done.

-          // 3. Not opened but other devices exists for this request -> remove
-          //    device from list, but don't signal an error.
-          devices->erase(device_it);
+          if (it->second.audio_devices.size()
+              + it->second.video_devices.size() <= 1) {
+            // 2. Device not opened and no other devices for this request ->
+            //    signal stream error and remove the request.
+            it->second.requester->StreamGenerationFailed(it->first);
+            requests_.erase(it);
+          } else {
+            // 3. Not opened but other devices exists for this request -> remove
+            //    device from list, but don't signal an error.
+            devices->erase(device_it);
+          }
         }
         return;
       }
@@ -686,8 +655,12 @@ void MediaStreamManager::DevicesFromRequest(
 bool MediaStreamManager::RequestDone(const DeviceRequest& request) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // Check if all devices are opened.
-  if (Requested(request.options,
-                content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE)) {
+  MediaStreamType stream_type = content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE;
+  if (Requested(request.options, stream_type)) {
+    if (request.state[stream_type] != DeviceRequest::kDone ||
+        request.state[stream_type] != DeviceRequest::kError)
+      return false;
+
     for (StreamDeviceInfoArray::const_iterator it =

[perkj_chrome, 2012/08/09 07:31:14]

Why do you need to check both the state and each device?
Seems better to check the state ? 

And the state is a vector. Isn't it enough to just loop through all the states
and don't care about the stream_type?

[no longer working on chromium, 2012/08/09 15:54:54]

I honestly never understand why we have the state and in_use flag. Maybe Magnus
and Wei can answer if it is enough to check the state here.

And we need to check the state, previously we can return true even though the
audio/video_devices is empty.

[mflodman_chromium_OOO, 2012/08/10 07:15:44]

Agree with Per. It should be enough to check the state for all types. It should
be done for stated Done, Error or if it's not requested.

          request.audio_devices.begin(); it != request.audio_devices.end();
          ++it) {
@@ -696,8 +669,13 @@ bool MediaStreamManager::RequestDone(const DeviceRequest& request) const {
       }
     }
   }
-  if (Requested(request.options,
-                content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE)) {
+
+  stream_type = content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE;
+  if (Requested(request.options, stream_type)) {
+    if (request.state[stream_type] == DeviceRequest::kDone ||
+        request.state[stream_type] == DeviceRequest::kError)
+          return false;
+
     for (StreamDeviceInfoArray::const_iterator it =
          request.video_devices.begin(); it != request.video_devices.end();

[perkj_chrome, 2012/08/09 07:31:14]

dito?

[no longer working on chromium, 2012/08/09 15:54:54]

Please see reply above.

          ++it) {