Make sure DeviceRequestMessageFilter cancel all pending EnumerateDevices requests

Make sure DeviceRequestMessageFilter call MediaStreamManager::CancelRequest for all pending requests to MediaStreamManager::EnumerateDevices when the IPC channel is teared down.

Before this change DeviceRequestMessageFilter call CancelRequest with the render process pid. But that pid was not been used as an identifier when calling EnumerateDevices. 

BUG=338561
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247744

[perkj_chrome, 2014-01-28 11:58:06]

Hej
Can you please review this bugfix?

[tommi (sloooow) - chröme, 2014-01-28 13:26:12]

https://codereview.chromium.org/144363005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/device_request_message_filter.cc
(right):

https://codereview.chromium.org/144363005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/device_request_message_filter.cc:134: this,
peer_pid(), -1, resource_context_, -1, MEDIA_DEVICE_AUDIO_CAPTURE,
is -1 ever a valid arguemnt to EnumerateDevices?  Should there be a DCHECK for
it in MediaStreamManager::EnumerateDevices? (possibly also inside
DeviceRequest?)

Also, why is it OK to pass -1 for the render_view_id?

[perkj_chrome, 2014-01-28 20:01:03]

PTAL

https://codereview.chromium.org/144363005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/device_request_message_filter.cc
(right):

https://codereview.chromium.org/144363005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/device_request_message_filter.cc:134: this,
peer_pid(), -1, resource_context_, -1, MEDIA_DEVICE_AUDIO_CAPTURE,
On 2014/01/28 13:26:12, tommi wrote:
> is -1 ever a valid arguemnt to EnumerateDevices?  Should there be a DCHECK for
> it in MediaStreamManager::EnumerateDevices? (possibly also inside
> DeviceRequest?)
> 
> Also, why is it OK to pass -1 for the render_view_id?

Added dcheck for render_process.
-1 is kind of ok. EnumerateDevice was written for pepper using
MEdiaStreamDispatcher which exist one per render view. The device message filter
exist one per render process.

None of this really matters in this case if we change the OnChannelClosing to
cancel all pending requests and don't use the process as identifier.

[vrk (LEFT CHROMIUM), 2014-01-28 20:50:42]

LGTM with nit

https://codereview.chromium.org/144363005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/device_request_message_filter.cc
(right):

https://codereview.chromium.org/144363005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/device_request_message_filter.cc:137: this,
peer_pid(), -1, resource_context_, -1, MEDIA_DEVICE_AUDIO_CAPTURE,
nit: You should no longer need to pass the peer_pid, and I actually think you
should keep this -1. The reason why I made these parameters -1 is because these
values aren't relevant for getSources. I believe process id + requesting view id
are only used to trigger the allow/deny info bar, which is not used for
getSources. I can't remember off the top of my head what |page_request_id| is
for. (Per or Tommi, do you know?)

If you want to pass the real process id, then I'd suggest you pass the real
requesting view id and page_request_id as well (i.e. either set all unused
values to -1, or no values should be -1). Otherwise your code implies that
process id is used and the others aren't, which isn't true -- they're all
equally meaningless to getSources :) 

The bigger problem, of course, is DeviceRequest and MediaStreamManager needs to
be refactored (crbug.com/334244) This is a perfect example of a flaw in the API
that needs fixing.

[vrk (LEFT CHROMIUM), 2014-01-28 21:00:30]

Another nit: update bug title+description

[vrk (LEFT CHROMIUM), 2014-01-28 21:00:59]

On 2014/01/28 21:00:30, Victoria Kirst wrote:
> Another nit: update bug title+description

Gahh, I meant "CL" title+description!

[perkj_chrome, 2014-01-29 07:43:36]

ptal

I would like to keep this as small as possible so I can get it merged.

[vrk (LEFT CHROMIUM), 2014-01-29 18:18:23]

lgtm++

[commit-bot: I haz the power, 2014-01-29 18:19:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/144363005/60001

[commit-bot: I haz the power, 2014-01-29 18:48:42]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-29 19:08:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/144363005/60001

[commit-bot: I haz the power, 2014-01-29 21:41:11]

Change committed as 247744