Make TabCapture requests use the target render process and render view id's for UI permissions.

Make TabCapture requests use the target render process and render view id's for UI permissions.

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

[justinlin, 2012-12-05 13:37:34]

Hi Shijing, these are the changes needed on top of your CL to keep TabCapture
working. Let me know if you want to me submit this separately, or if you wanted
to just patch it in. Thanks.

[no longer working on chromium, 2012-12-05 17:38:37]

HI Justin,

Please take a look at the comments. I think both ways work for me, but if a
bigger refactoring is needed, we might want to do it in a separate CL.

BR,
SX

https://codereview.chromium.org/11451006/diff/2001/chrome/browser/media/media...
File chrome/browser/media/media_stream_devices_controller.cc (right):

https://codereview.chromium.org/11451006/diff/2001/chrome/browser/media/media...
chrome/browser/media/media_stream_devices_controller.cc:64: "%i:%i",
request_.render_process_id, request_.render_view_id);
this code looks a bit odd, can you consider changeing the type to 
typedef std::map<std::pair<int, int>,  TabCaptureRequest>
DeviceCaptureRequestMap;

https://codereview.chromium.org/11451006/diff/2001/content/browser/renderer_h...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/2001/content/browser/renderer_h...
content/browser/renderer_host/media/media_stream_manager.cc:252:
DCHECK(WebContentsCaptureUtil::ExtractTabCaptureTarget(
If you put it in DCHECK, the code won't run in release mode.
CHECK ?

[justinlin, 2012-12-06 06:03:51]

Thanks Shijing, addressed the comments.

aa@: PTAL as well, mostly just cleanup.

On 2012/12/05 17:38:37, xians1 wrote:
> HI Justin,
> 
> Please take a look at the comments. I think both ways work for me, but if a
> bigger refactoring is needed, we might want to do it in a separate CL.
> 
> BR,
> SX
> 
>
https://codereview.chromium.org/11451006/diff/2001/chrome/browser/media/media...
> File chrome/browser/media/media_stream_devices_controller.cc (right):
> 
>
https://codereview.chromium.org/11451006/diff/2001/chrome/browser/media/media...
> chrome/browser/media/media_stream_devices_controller.cc:64: "%i:%i",
> request_.render_process_id, request_.render_view_id);
> this code looks a bit odd, can you consider changeing the type to 
> typedef std::map<std::pair<int, int>,  TabCaptureRequest>
> DeviceCaptureRequestMap;
> 
>
https://codereview.chromium.org/11451006/diff/2001/content/browser/renderer_h...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
https://codereview.chromium.org/11451006/diff/2001/content/browser/renderer_h...
> content/browser/renderer_host/media/media_stream_manager.cc:252:
> DCHECK(WebContentsCaptureUtil::ExtractTabCaptureTarget(
> If you put it in DCHECK, the code won't run in release mode.
> CHECK ?

[justinlin, 2012-12-06 06:05:13]

xians@: Note that I rebased off of master so I can land this separately. I think
there's slight changes that need to be made so it applies to your patch, like in
patch set 1. PTAL, thanks.

On 2012/12/06 06:03:51, justinlin wrote:
> Thanks Shijing, addressed the comments.
> 
> aa@: PTAL as well, mostly just cleanup.
> 
> On 2012/12/05 17:38:37, xians1 wrote:
> > HI Justin,
> > 
> > Please take a look at the comments. I think both ways work for me, but if a
> > bigger refactoring is needed, we might want to do it in a separate CL.
> > 
> > BR,
> > SX
> > 
> >
>
https://codereview.chromium.org/11451006/diff/2001/chrome/browser/media/media...
> > File chrome/browser/media/media_stream_devices_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/11451006/diff/2001/chrome/browser/media/media...
> > chrome/browser/media/media_stream_devices_controller.cc:64: "%i:%i",
> > request_.render_process_id, request_.render_view_id);
> > this code looks a bit odd, can you consider changeing the type to 
> > typedef std::map<std::pair<int, int>,  TabCaptureRequest>
> > DeviceCaptureRequestMap;
> > 
> >
>
https://codereview.chromium.org/11451006/diff/2001/content/browser/renderer_h...
> > File content/browser/renderer_host/media/media_stream_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/11451006/diff/2001/content/browser/renderer_h...
> > content/browser/renderer_host/media/media_stream_manager.cc:252:
> > DCHECK(WebContentsCaptureUtil::ExtractTabCaptureTarget(
> > If you put it in DCHECK, the code won't run in release mode.
> > CHECK ?

[no longer working on chromium, 2012-12-07 14:13:10]

Nice work.
lgtm with some nits.

https://codereview.chromium.org/11451006/diff/4/chrome/browser/extensions/api...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/11451006/diff/4/chrome/browser/extensions/api...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:45:
std::pair<int, int> id_pair = std::make_pair(render_process_id,
nit, use key instead

https://codereview.chromium.org/11451006/diff/4/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/4/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:255:
CHECK(WebContentsCaptureUtil::ExtractTabCaptureTarget(
what will trigger this GenerateStreamForDevice?
If this can be triggered by JS, you should not add CHECK here, since the hackers
can easily crash the chrome.

https://codereview.chromium.org/11451006/diff/4/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:875:
device_info.device_id = request->requested_device_id;
curiously, what is this device_id? is the process_id + view_id? or is it a real
device?

[justinlin, 2012-12-07 15:50:09]

Thanks for the review! Added small fix in media_stream_dispatcher_host.cc to
remove a DCHECK and handle if only an audio device_id is passed.

Will probably TBR this since the extension side changes are just a minor
cleanup.

https://codereview.chromium.org/11451006/diff/4/chrome/browser/extensions/api...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/11451006/diff/4/chrome/browser/extensions/api...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:45:
std::pair<int, int> id_pair = std::make_pair(render_process_id,
On 2012/12/07 14:13:10, xians1 wrote:
> nit, use key instead

Done.

https://codereview.chromium.org/11451006/diff/4/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/4/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:255:
CHECK(WebContentsCaptureUtil::ExtractTabCaptureTarget(
On 2012/12/07 14:13:10, xians1 wrote:
> what will trigger this GenerateStreamForDevice?
> If this can be triggered by JS, you should not add CHECK here, since the
hackers
> can easily crash the chrome.

Done. Good point. Fixed and tested this possible issue.

https://codereview.chromium.org/11451006/diff/4/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.cc:875:
device_info.device_id = request->requested_device_id;
On 2012/12/07 14:13:10, xians1 wrote:
> curiously, what is this device_id? is the process_id + view_id? or is it a
real
> device?

It's 'process_id + view_id'. We need to pass it to the WebContentsVideoDevice
and eventually to the VirtualAudio streams too (right now for audio, we just use
whole browser audio, so it's not used) to know which tab to capture.

[no longer working on chromium, 2012-12-07 15:56:39]

https://codereview.chromium.org/11451006/diff/12002/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/12002/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:255: bool
has_valid_device_id = WebContentsCaptureUtil::ExtractTabCaptureTarget(
How about doing an early return with a std::string() if
(WebContentsCaptureUtil::ExtractTabCaptureTarget)

[justinlin, 2012-12-07 15:59:52]

https://codereview.chromium.org/11451006/diff/12002/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/12002/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:255: bool
has_valid_device_id = WebContentsCaptureUtil::ExtractTabCaptureTarget(
On 2012/12/07 15:56:39, xians1 wrote:
> How about doing an early return with a std::string() if
> (WebContentsCaptureUtil::ExtractTabCaptureTarget)

I considered that, but is that safe to do? If we do that, it will fail the
DCHECK in media_stream_dispatcher_host. I remember wjia suggested in one of my
earlier reviews that I create and cancel the request like this so there is a
common way to discard the request.

[no longer working on chromium, 2012-12-07 16:06:06]

https://codereview.chromium.org/11451006/diff/12002/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/12002/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:255: bool
has_valid_device_id = WebContentsCaptureUtil::ExtractTabCaptureTarget(
On 2012/12/07 15:59:53, justinlin wrote:
> On 2012/12/07 15:56:39, xians1 wrote:
> > How about doing an early return with a std::string() if
> > (WebContentsCaptureUtil::ExtractTabCaptureTarget)
> 
> I considered that, but is that safe to do? If we do that, it will fail the
> DCHECK in media_stream_dispatcher_host. I remember wjia suggested in one of my
> earlier reviews that I create and cancel the request like this so there is a
> common way to discard the request.

It sounds wrong since the requests can fail on creation.
I can make a CL to correct the code in media_stream_dispatcher_host.cc

[justinlin, 2012-12-07 17:06:39]

Thanks

Matt, did you want to take a look?

https://codereview.chromium.org/11451006/diff/12002/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/12002/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:255: bool
has_valid_device_id = WebContentsCaptureUtil::ExtractTabCaptureTarget(
On 2012/12/07 16:06:07, xians1 wrote:
> On 2012/12/07 15:59:53, justinlin wrote:
> > On 2012/12/07 15:56:39, xians1 wrote:
> > > How about doing an early return with a std::string() if
> > > (WebContentsCaptureUtil::ExtractTabCaptureTarget)
> > 
> > I considered that, but is that safe to do? If we do that, it will fail the
> > DCHECK in media_stream_dispatcher_host. I remember wjia suggested in one of
my
> > earlier reviews that I create and cancel the request like this so there is a
> > common way to discard the request.
> 
> It sounds wrong since the requests can fail on creation.
> I can make a CL to correct the code in media_stream_dispatcher_host.cc

OK, changed it to match the CL you just sent. This does seem better. I'll wait
for your CL to land first.

[Matt Perry, 2012-12-07 20:17:36]

lgtm

[wjia(left Chromium), 2012-12-08 20:09:40]

https://codereview.chromium.org/11451006/diff/18002/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/18002/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:266: return
std::string();
drive by. I am wondering the consequence of returning an empty label. Even
though MediaStreamDispatcherHost could be changed to allow empty label, but what
about the requester in renderer process? Will it wait for some kind of response
from browser process for its request of GenerateStreamForDevice? If that's the
case, MediaStreamDispatcherHost will never send a response to renderer process
because it don't create a request at all.

So, shall we use NOTREACHER() or something else here?

[justinlin, 2012-12-08 23:30:27]

shijing/wjia ptal. Changed it back to a previous approach for
media_stream_manager.cc. Small change that we skip the PostRequestToUI() if we
need to cancel the request anyway. Please CQ if this look ok, thanks.

https://codereview.chromium.org/11451006/diff/18002/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/18002/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:266: return
std::string();
On 2012/12/08 20:09:40, wjia wrote:
> drive by. I am wondering the consequence of returning an empty label. Even
> though MediaStreamDispatcherHost could be changed to allow empty label, but
what
> about the requester in renderer process? Will it wait for some kind of
response
> from browser process for its request of GenerateStreamForDevice? If that's the
> case, MediaStreamDispatcherHost will never send a response to renderer process
> because it don't create a request at all.
> 
> So, shall we use NOTREACHER() or something else here?

Ah right. Actually, I just looked, and media_stream_dispatcher.cc stores a
Request object, so it expects a reply eventually, so we need to cancel the
request. I'll change it back to the original approach.

We can't put NOTREACHED() because it could end up in that branch through user
input.

[commit-bot: I haz the power, 2012-12-10 05:17:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/11451006/30001

[wjia(left Chromium), 2012-12-10 05:34:18]

https://codereview.chromium.org/11451006/diff/30001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/11451006/diff/30001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:278:
base::Unretained(this), label));
In case of error, browser process should send an error message to renderer
process, instead of just canceling the request. You can address this in a
separate patch.

[commit-bot: I haz the power, 2012-12-10 08:53:06]

Change committed as 172027

[no longer working on chromium, 2012-12-10 10:03:18]

https://chromiumcodereview.appspot.com/11451006/diff/18002/content/browser/re...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/11451006/diff/18002/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:266: return
std::string();
On 2012/12/08 23:30:27, justinlin wrote:
> On 2012/12/08 20:09:40, wjia wrote:
> > drive by. I am wondering the consequence of returning an empty label. Even
> > though MediaStreamDispatcherHost could be changed to allow empty label, but
> what
> > about the requester in renderer process? Will it wait for some kind of
> response
> > from browser process for its request of GenerateStreamForDevice? If that's
the
> > case, MediaStreamDispatcherHost will never send a response to renderer
process
> > because it don't create a request at all.
> > 
> > So, shall we use NOTREACHER() or something else here?
> 
> Ah right. Actually, I just looked, and media_stream_dispatcher.cc stores a
> Request object, so it expects a reply eventually, so we need to cancel the
> request. I'll change it back to the original approach.
> 
> We can't put NOTREACHED() because it could end up in that branch through user
> input.

I am trying to understand more. Justin, what kind of reply does the
dispatcher.cc expect? And does the render client use the label to cancel the
request?

https://chromiumcodereview.appspot.com/11451006/diff/30001/content/browser/re...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://chromiumcodereview.appspot.com/11451006/diff/30001/content/browser/re...
content/browser/renderer_host/media/media_stream_manager.cc:278:
base::Unretained(this), label));
On 2012/12/10 05:34:18, wjia wrote:
> In case of error, browser process should send an error message to renderer
> process, instead of just canceling the request. You can address this in a
> separate patch.

+1, it seems to me we create a request here and does nothing.
Please note that if the render client does not cancel the request, the request
will leak.

[justinlin, 2012-12-10 11:19:54]

Thank you for the comments Wei and Shijing. I see the issue now. I thought
CancelRequest would cancel the request in media_stream_dispatcher_host and on
the renderer side too, but it doesn't.. This was a pre-existing issue then..
Nothing will leak (DeviceRequest is cleaned up in CancelRequest), but the
Request object in the dispatcher/dispatcher_host will be left dangling. They are
in lists of objects though, so they'll get cleaned up on destruction.
Sent a CL to fix this. Sorry for missing this..

On 2012/12/10 10:03:18, xians1 wrote:
>
https://chromiumcodereview.appspot.com/11451006/diff/18002/content/browser/re...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11451006/diff/18002/content/browser/re...
> content/browser/renderer_host/media/media_stream_manager.cc:266: return
> std::string();
> On 2012/12/08 23:30:27, justinlin wrote:
> > On 2012/12/08 20:09:40, wjia wrote:
> > > drive by. I am wondering the consequence of returning an empty label. Even
> > > though MediaStreamDispatcherHost could be changed to allow empty label,
but
> > what
> > > about the requester in renderer process? Will it wait for some kind of
> > response
> > > from browser process for its request of GenerateStreamForDevice? If that's
> the
> > > case, MediaStreamDispatcherHost will never send a response to renderer
> process
> > > because it don't create a request at all.
> > > 
> > > So, shall we use NOTREACHER() or something else here?
> > 
> > Ah right. Actually, I just looked, and media_stream_dispatcher.cc stores a
> > Request object, so it expects a reply eventually, so we need to cancel the
> > request. I'll change it back to the original approach.
> > 
> > We can't put NOTREACHED() because it could end up in that branch through
user
> > input.
> 
> I am trying to understand more. Justin, what kind of reply does the
> dispatcher.cc expect? And does the render client use the label to cancel the
> request?
> 
>
https://chromiumcodereview.appspot.com/11451006/diff/30001/content/browser/re...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11451006/diff/30001/content/browser/re...
> content/browser/renderer_host/media/media_stream_manager.cc:278:
> base::Unretained(this), label));
> On 2012/12/10 05:34:18, wjia wrote:
> > In case of error, browser process should send an error message to renderer
> > process, instead of just canceling the request. You can address this in a
> > separate patch.
> 
> +1, it seems to me we create a request here and does nothing.
> Please note that if the render client does not cancel the request, the request
> will leak.