Resolve the problems where we can leak the system tray UI

content/browser/renderer_host/media/media_stream_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/media_stream_manager.h"
 
 #include <list>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 160 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   // Create a new request based on options.
   DeviceRequest new_request(requester, options,
                             render_process_id,
                             render_view_id,
                             security_origin);
   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));
 
   DeviceRequests::iterator it = requests_.find(label);
   if (it != requests_.end()) {
     // The request isn't complete.
     if (!RequestDone(it->second)) {
       DeviceRequest* request = &(it->second);
       if (request->state[content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE] ==
           DeviceRequest::kOpening) {
@@ skipping 27 matching lines @@
     for (StreamDeviceInfoArray::iterator audio_it =
          it->second.audio_devices.begin();
          audio_it != it->second.audio_devices.end(); ++audio_it) {
       audio_input_device_manager()->Close(audio_it->session_id);
     }
     for (StreamDeviceInfoArray::iterator video_it =
          it->second.video_devices.begin();
          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);
     return;
   }
 }
 
 void MediaStreamManager::EnumerateDevices(
     MediaStreamRequester* requester,
     int render_process_id,
@@ skipping 73 matching lines @@
   do {
     request_label = RandomLabel();
   } while (requests_.find(request_label) != requests_.end());
 
   requests_.insert(std::make_pair(request_label, *new_request));
 
   // Get user confirmation to use capture devices.
   // 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;
 }
 
 void MediaStreamManager::EnsureDeviceThreadAndListener() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (device_thread_.get())
     return;
 
@@ skipping 117 matching lines @@
             request.state[device_it->stream_type] = DeviceRequest::kOpening;
             if (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE)
               request.audio_devices.push_back(device);
             else
               request.video_devices.push_back(device);
             break;
           }
         }
         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();
   enumeration_in_progress_[stream_type] = false;
 }
 
 void MediaStreamManager::Error(MediaStreamType stream_type,
                                int capture_session_id,
                                MediaStreamProviderError error) {
   // Find the device for the error call.
@@ skipping 18 matching lines @@
           // 1. Already opened -> signal device failure and close device.
           //    Use device_idx to signal which of the devices encountered an
           //    error.
           if (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE) {
             it->second.requester->AudioDeviceFailed(it->first, device_idx);
           } else if (stream_type ==
                      content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE) {
             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;
       }
     }
   }
 }
 
 void MediaStreamManager::DevicesAccepted(const std::string& label,
                                          const StreamDeviceInfoArray& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
@@ skipping 118 matching lines @@
         content::MediaStreamDevice(
             content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE,
             it->device_id,
             it->name));
   }
 }
 
 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) {
       if (it->in_use == false) {
         return false;
       }
     }
   }
-  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) {
       if (it->in_use == false) {
         return false;
       }
     }
   }
   return true;
 }
 
 // Called to get media capture device manager of specified type.
 MediaStreamProvider* MediaStreamManager::GetDeviceManager(
     MediaStreamType stream_type) {
   if (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE) {
     return video_capture_manager();
   } else if (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE) {
     return audio_input_device_manager();
   }
   NOTREACHED();
   return NULL;
 }
 
 }  // namespace media_stream