Resolve the problems where we can leak the system tray UI

content/browser/renderer_host/media/media_stream_device_settings.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_device_settings.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 81 matching lines @@
       int render_pid,
       int render_vid,
       const GURL& origin,
       const StreamOptions& request_options)
       : MediaStreamRequest(render_pid, render_vid, origin),
         options(request_options),
         posted_task(false) {}
 
   ~MediaStreamDeviceSettingsRequest() {}
 
+  bool IsReadyForView() {
+    if (options.audio &&
+        !devices_full.count(content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE))

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

Since it is counting, I would prefer '== 0' rather than '!'.

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

Hope this is fine.

+      return false;
+
+    if (options.video &&
+        !devices_full.count(content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE))
+      return false;
+
+    // We have got all the requested devices, it is ready if it has not
+    // been posted for UI.
+    return !posted_task;
+  }
+
   // Request options.
   StreamOptions options;

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

options_

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

Done.

   // Map containing available devices for the requested capture types.
+  // Note, never call devices_full[stream_type] before making sure the entry

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

nit- this comment seems irrelevant in this class

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

It is relevant. One of the bug we are fixing is introduced by calling
devices_full_[type].empty() which creates an empty entry on the map.

+  // has been created.
   DeviceMap devices_full;

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

device_full_

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

Done.

   // Whether or not a task was posted to make the call to
   // RequestMediaAccessPermission, to make sure that we never post twice to it.
   bool posted_task;

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

posted_task_

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

Done.

 };
 
 namespace {
 
 // Sends the request to the appropriate WebContents.
 void DoDeviceRequest(
     const MediaStreamDeviceSettingsRequest& request,
     const content::MediaResponseCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ skipping 22 matching lines @@
 }
 
 MediaStreamDeviceSettings::~MediaStreamDeviceSettings() {
   STLDeleteValues(&requests_);
 }
 
 void MediaStreamDeviceSettings::RequestCaptureDeviceUsage(
     const std::string& label, int render_process_id, int render_view_id,
     const StreamOptions& request_options, const GURL& security_origin) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   if (requests_.find(label) != requests_.end()) {

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

This should never happen, can switch to a DCHECK instead?

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

Good point. Done

     // Request with this id already exists.
     requester_->SettingsError(label);
     return;
   }
 
   // Create a new request.
   requests_.insert(std::make_pair(label, new MediaStreamDeviceSettingsRequest(
       render_process_id, render_view_id, security_origin, request_options)));
 }
 
 void MediaStreamDeviceSettings::RemovePendingCaptureRequest(
     const std::string& label) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   SettingsRequests::iterator request_it = requests_.find(label);
   if (request_it != requests_.end()) {
-    // Proceed the next pending request for the same page.
     MediaStreamDeviceSettingsRequest* request = request_it->second;
-    std::string new_label = FindReadyRequestForView(request->render_view_id,
-                                                    request->render_process_id);
-    if (!new_label.empty()) {
-      PostRequestToUi(new_label);
-    }
-
+    int render_view_id = request->render_view_id;
+    int render_process_id = request->render_process_id;
     // TODO(xians): Post a cancel request on UI thread to dismiss the infobar

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

Is this todo rellevant still? Sounds important.

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

No, they are not relevant to the issues we are fixing here. This should be
addressed by a separate CL.

     // if request has been sent to the UI.
     // Remove the request from the queue.
     requests_.erase(request_it);
     delete request;
+
+    // Proceed the next pending request for the same page.
+    std::string new_label = FindReadyRequestForView(render_view_id,

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

This should only be done if the removed request has been posted to the ui, to
avoid posting two infobars at the same time.

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

Done.

+                                                    render_process_id);
+    if (!new_label.empty()) {
+      PostRequestToUi(new_label);
+    }
   }
 }
 
 void MediaStreamDeviceSettings::AvailableDevices(
     const std::string& label,
     MediaStreamType stream_type,
     const StreamDeviceInfoArray& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   SettingsRequests::iterator request_it = requests_.find(label);
   DCHECK(request_it != requests_.end());
-
   // Add the answer for the request.
   MediaStreamDeviceSettingsRequest* request = request_it->second;
   DCHECK_EQ(request->devices_full.count(stream_type), static_cast<size_t>(0)) <<
       "This request already has a list of devices for this stream type.";
   request->devices_full[stream_type] = devices;
 
-  // Check if we're done.
-  size_t num_media_requests = 0;
-  if (request->options.audio) {
-    num_media_requests++;
-  }
-  if (request->options.video) {
-    num_media_requests++;
-  }
-
-  if (request->devices_full.size() == num_media_requests) {
+   if (request->IsReadyForView()) {
     // We have all answers needed.
     if (!use_fake_ui_) {
       // Abort if the task was already posted: wait for it to PostResponse.
       if (request->posted_task) {
         return;
       }
       // Since the UI can only handle one request at the time, verify there
       // is no unanswered request posted for this view. If there is, this
       // new request will be handled once we get a response for the first one.
       if (IsUiBusy(request->render_view_id, request->render_process_id)) {
         return;
       }
       PostRequestToUi(label);
     } else {
       // Used to fake UI, which is needed for server based testing.

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

Can this whole else case be moved to a new function ie 
if (use_fake_ui) {
 HandleFakeUi
 return;
}
...
no normal handling here.

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

+1, this would be nice to hide.

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

Done.

       // Choose first non-opened device for each media type.
       StreamDeviceInfoArray devices_to_use;
       for (DeviceMap::iterator it = request->devices_full.begin();
            it != request->devices_full.end(); ++it) {
-        for (StreamDeviceInfoArray::iterator device_it = it->second.begin();
-             device_it != it->second.end(); ++device_it) {
+        StreamDeviceInfoArray::iterator device_it = it->second.begin();
+        for (; device_it != it->second.end(); ++device_it) {
           if (!device_it->in_use) {
             devices_to_use.push_back(*device_it);
             break;
           }
         }
-      }
 
-      if (!request->devices_full[
-              content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE].empty() &&
-          num_media_requests != devices_to_use.size()) {
         // Not all requested device types were opened. This happens if all
         // video capture devices are already opened, |in_use| isn't set for

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

Is this comment valid? Can't you open the same device multiple times now?

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

This was to be able to open several devices before the UI existed and there were
any selection. It should be moved inside the if below, and possibly rewritten,
considering the new code.

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

No idea if the comment is valid or not. If will be great if someone can verify
it and tell me how this should be changed.

Done with moving the comments into the if case.

         // audio devices. Allow the first video capture device in the list to be
         // opened for this user too.
-        StreamDeviceInfoArray device_array =
-            request->devices_full[
-                content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE];
-        devices_to_use.push_back(*(device_array.begin()));
+        if (it->second.begin() != it->second.end() &&

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

How about using a temp variable above instead of it? 
Ie
const DeviceMap& device_map = it->second;
and here
if (!device_map.empty() && device_it == device_map.end()) {
...

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

How about it->second.size() != 0 && device_it == it->second.end()
It fits in one line and does not need an temp variable.

and for some unknown reason empty() behaves differently with .size(), using it
destroys the logic and media_stream_dispatcher_host_unittests failed by hitting
a DCHECK.

+            device_it == it->second.end()) {
+          devices_to_use.push_back(*it->second.begin());
+        }
       }
 
       // Post result and delete request.
       requester_->DevicesAccepted(label, devices_to_use);
       requests_.erase(request_it);
       delete request;
     }
   }
 }
 
 void MediaStreamDeviceSettings::PostResponse(
     const std::string& label,
     const content::MediaStreamDevices& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   SettingsRequests::iterator req = requests_.find(label);
   // Return if the request has been removed.
   if (req == requests_.end())
     return;
 
   DCHECK(requester_);
   MediaStreamDeviceSettingsRequest* request = req->second;
   requests_.erase(req);
 
   // Look for queued requests for the same view. If there is a pending request,
@@ skipping 41 matching lines @@
 }
 
 std::string MediaStreamDeviceSettings::FindReadyRequestForView(
     int render_view_id, int render_process_id) {
   for (SettingsRequests::iterator it = requests_.begin(); it != requests_.end();
        ++it) {
     if (it->second->render_process_id == render_process_id &&
         it->second->render_view_id == render_view_id) {
       // This request belongs to the given render view.
       MediaStreamDeviceSettingsRequest* request = it->second;
-      if (request->options.audio &&
-          request->devices_full[
-              content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE].empty()) {
-        // Audio requested, but no devices enumerated yet. Continue to next
-        // request.
-        continue;
-      }
-      if (request->options.video &&
-          request->devices_full[
-              content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE].empty()) {
-        // Video requested, but no devices enumerated yet. Continue to next
-        // request.
-        continue;
-      }
-      // This request belongs to the same view as the treated request and is
-      // ready to be requested. Return its label.
-      return it->first;
+      if (request->IsReadyForView())
+        return it->first;
     }
   }
   return std::string();
 }
 
 void MediaStreamDeviceSettings::PostRequestToUi(const std::string& label) {
   MediaStreamDeviceSettingsRequest* request = requests_[label];
   DCHECK(request != NULL);
 
   request->posted_task = true;
@@ skipping 14 matching lines @@
   content::MediaResponseCallback callback =
       base::Bind(&ResponseCallbackHelper::PostResponse,
                  helper.get(), label);
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&DoDeviceRequest, *request, callback));
 }
 
 }  // namespace media_stream