Resolve the problems where we can leak the system tray UI

content/browser/renderer_host/media/media_stream_device_settings.cc

Index: content/browser/renderer_host/media/media_stream_device_settings.cc
diff --git a/content/browser/renderer_host/media/media_stream_device_settings.cc b/content/browser/renderer_host/media/media_stream_device_settings.cc
index f701a1d179793d8033e550ccdda8888c8b8b0423..b8836aae9898122b1241b1e4eb2f09efd4321fdb 100644
--- a/content/browser/renderer_host/media/media_stream_device_settings.cc
+++ b/content/browser/renderer_host/media/media_stream_device_settings.cc
@@ -94,18 +94,35 @@ class MediaStreamDeviceSettingsRequest : public MediaStreamRequest {
       const GURL& origin,
       const StreamOptions& request_options)
       : MediaStreamRequest(render_pid, render_vid, origin),
-        options(request_options),
-        posted_task(false) {}
+        options_(request_options),
+        posted_task_(false) {}
 
   ~MediaStreamDeviceSettingsRequest() {}
 
+  bool IsReadyForView() {
+    if (options_.audio && devices_full_.count(
+            content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE) == 0)
+      return false;
+
+    if (options_.video && devices_full_.count(
+            content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE) == 0)
+      return false;

[wjia(left Chromium), 2012/08/10 01:23:13]

combine both if above.

[no longer working on chromium, 2012/08/10 11:50:56]

Done.

+
+    // 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;
+  StreamOptions options_;
   // Map containing available devices for the requested capture types.
-  DeviceMap devices_full;
+  // Note, never call devices_full_[stream_type].empty() before making sure
+  // that type of device has existed on the map, otherwise it will create an
+  // empty device entry on the map.
+  DeviceMap devices_full_;
   // 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;
+  bool posted_task_;
 };
 
 namespace {
@@ -148,12 +165,7 @@ 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()) {
-    // Request with this id already exists.
-    requester_->SettingsError(label);
-    return;
-  }
+  DCHECK(requests_.find(label) == requests_.end());
 
   // Create a new request.
   requests_.insert(std::make_pair(label, new MediaStreamDeviceSettingsRequest(
@@ -163,22 +175,29 @@ void MediaStreamDeviceSettings::RequestCaptureDeviceUsage(
 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;
+    bool was_posted = request->posted_task_;
     // TODO(xians): Post a cancel request on UI thread to dismiss the infobar
     // if request has been sent to the UI.
     // Remove the request from the queue.
     requests_.erase(request_it);
     delete request;
+
+    // Simply return if the canceled request has not been brought to UI.
+    if (!was_posted)
+      return;
+
+    // Proceed the next pending request to replace the old infobar on the same
+    // page.
+    std::string new_label = FindReadyRequestForView(render_view_id,
+                                                    render_process_id);
+    if (!new_label.empty()) {
+      PostRequestToUi(new_label);
+    }
   }
 }
 
@@ -190,29 +209,15 @@ void MediaStreamDeviceSettings::AvailableDevices(
 
   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++;
-  }
+  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;
 
-  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.
@@ -221,37 +226,7 @@ void MediaStreamDeviceSettings::AvailableDevices(
       }
       PostRequestToUi(label);
     } else {
-      // Used to fake UI, which is needed for server based testing.
-      // 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) {
-          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
-        // 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()));
-      }
-
-      // Post result and delete request.
-      requester_->DevicesAccepted(label, devices_to_use);
-      requests_.erase(request_it);
-      delete request;
+      PostRequestToFakeUI(label);

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

nit: usually you should do the short exit first.
ie 
if(use_fake_ui) {
 PostRequestToFakeUI
 return;
}
if (IsUiBusy( ....
...


 

[no longer working on chromium, 2012/08/10 11:50:56]

Done.

     }
   }
 }
@@ -260,14 +235,13 @@ 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;
+  scoped_ptr<MediaStreamDeviceSettingsRequest> request(req->second);
   requests_.erase(req);
 
   // Look for queued requests for the same view. If there is a pending request,
@@ -283,8 +257,8 @@ void MediaStreamDeviceSettings::PostResponse(
     StreamDeviceInfoArray deviceList;
     for (content::MediaStreamDevices::const_iterator dev = devices.begin();
          dev != devices.end(); ++dev) {
-      DeviceMap::iterator subList = request->devices_full.find(dev->type);
-      DCHECK(subList != request->devices_full.end());
+      DeviceMap::iterator subList = request->devices_full_.find(dev->type);
+      DCHECK(subList != request->devices_full_.end());
 
       deviceList.push_back(*std::find_if(subList->second.begin(),
           subList->second.end(), DeviceIdEquals(dev->device_id)));
@@ -293,7 +267,6 @@ void MediaStreamDeviceSettings::PostResponse(
   } else {
     requester_->SettingsError(label);
   }
-  delete request;
 }
 
 void MediaStreamDeviceSettings::UseFakeUI() {
@@ -307,7 +280,7 @@ bool MediaStreamDeviceSettings::IsUiBusy(int render_view_id,
        it != requests_.end(); ++it) {
     if (it->second->render_process_id == render_process_id &&
         it->second->render_view_id == render_view_id &&
-        it->second->posted_task) {
+        it->second->posted_task_) {

[wjia(left Chromium), 2012/08/10 01:23:13]

this looks weird, some members have "_" while others don't. it looks like
MediaStreamDeviceSettingsRequest should be struct, instead of class.

[no longer working on chromium, 2012/08/10 11:50:56]

Done.

       return true;
     }
   }
@@ -322,23 +295,8 @@ std::string MediaStreamDeviceSettings::FindReadyRequestForView(
         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();
@@ -348,11 +306,11 @@ void MediaStreamDeviceSettings::PostRequestToUi(const std::string& label) {
   MediaStreamDeviceSettingsRequest* request = requests_[label];
   DCHECK(request != NULL);
 
-  request->posted_task = true;
+  request->posted_task_ = true;
 
   // Create the simplified list of devices.
-  for (DeviceMap::iterator it = request->devices_full.begin();
-       it != request->devices_full.end(); ++it) {
+  for (DeviceMap::iterator it = request->devices_full_.begin();
+       it != request->devices_full_.end(); ++it) {
     request->devices[it->first].clear();
     for (StreamDeviceInfoArray::iterator device = it->second.begin();
         device != it->second.end(); ++device) {
@@ -372,4 +330,43 @@ void MediaStreamDeviceSettings::PostRequestToUi(const std::string& label) {
       base::Bind(&DoDeviceRequest, *request, callback));
 }
 
+void MediaStreamDeviceSettings::PostRequestToFakeUI(const std::string& label) {
+  SettingsRequests::iterator request_it = requests_.find(label);
+  DCHECK(request_it != requests_.end());
+  MediaStreamDeviceSettingsRequest* request = request_it->second;
+
+  // Used to fake UI, which is needed for server based testing.
+  // Choose first non-opened device for each media type.
+  content::MediaStreamDevices devices_to_use;
+  for (DeviceMap::iterator it = request->devices_full_.begin();
+      it != request->devices_full_.end(); ++it) {

[wjia(left Chromium), 2012/08/10 01:23:13]

indent.

[no longer working on chromium, 2012/08/10 11:50:56]

Done.

+    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(content::MediaStreamDevice(
+            device_it->stream_type, device_it->device_id, device_it->name));
+        break;
+      }
+    }
+
+    if (it->second.size() != 0 &&
+        device_it == it->second.end()) {
+      // Not all requested device types were opened. This happens if all
+      // video capture devices are already opened, |in_use| isn't set for

[wjia(left Chromium), 2012/08/10 01:23:13]

is this comment still valid?

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

Think it should be removed. Or explained in a general since it's now general and
not video specific. Right?

[no longer working on chromium, 2012/08/10 11:50:56]

Done.

+      // audio devices. Allow the first video capture device in the list to be
+      // opened for this user too.
+      devices_to_use.push_back(
+          content::MediaStreamDevice(it->second.begin()->stream_type,
+                                     it->second.begin()->device_id,
+                                     it->second.begin()->name));
+    }
+  }
+
+  // Post result and delete request.

[wjia(left Chromium), 2012/08/10 01:23:13]

ditto.

[no longer working on chromium, 2012/08/10 11:50:56]

Done.

+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&MediaStreamDeviceSettings::PostResponse,
+                 weak_ptr_factory_.GetWeakPtr(), label, devices_to_use));

[wjia(left Chromium), 2012/08/10 01:23:13]

is this safe? POstResponse will call PostRequestToUi in some cases.

[no longer working on chromium, 2012/08/10 11:50:56]

Thanks for pointing out. we need to handle the fakeUI case in PostResponse. I
addressed this in the new unittest.

+}
+
 }  // namespace media_stream