Fix Task Manager to correctly display network usage of plug-in processes.

Fix Task Manager to correctly display network usage of plug-in processes.

BUG=chromium-os:2954
TEST=Run Chrome and open Task Manager, then play a video in YouTube and check whether the Network usage is correctly reported against the plug-in.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73884

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73915

[Wez, 2011-01-26 02:35:16]

One-line fix to let Task Manager correctly display network activity reported by
plug-ins.

[awong, 2011-01-27 21:03:07]

http://codereview.chromium.org/6328010/diff/3001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6328010/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_dispatcher_host.cc:534: // render process
through which the request was received.
drive-by style nit.

Add a newline before the TODO.  So 

// render process through which the request was received.
//
// TODO(wez): This lookup is too expensive - need a FromPID()...

[Wez, 2011-01-27 21:06:45]

The second patch-list attempts to fix Task Manager's reporting of network
resource (see chromium-os:2954) usage by plug-ins (and in principle any other
non-renderer processes) by:

1.  Referring to the originator by child-ID of the originator rather than
process-ID.
2.  Fixing up Task Manager's ResourceProvider interface and implementations to
match.

To get the originator's child-ID requires that
ResourceDispatcherHost::BeginRequest() matches the originating PID to the
relevant BrowserChildProcessHost instance - that is currently done by
enumerating the instances until a match is found, which may be too slow?

Because RenderProcessHost instances are not BrowserChildProcessHosts, they don't
get enumerated - they live on the UI thread anyway, so that avoids concurrency
issues - so if no match is found then the renderer child-ID supplied in the
request is used as the originator child-ID.  This also means that if a PID can't
be matched, we at least claim it originates from the right tab.

There's also some minor cpplint correction in one of the files.

[brettw, 2011-01-27 21:23:51]

Sorry I don't have time to think about this patch more at the moment. Please
ping me later if you want to chat more.

http://codereview.chromium.org/6328010/diff/3001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6328010/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_dispatcher_host.cc:537: for
(BrowserChildProcessHost::Iterator i; !i.Done(); ++i) {
I don't think this approach is a good idea. It's both slow, not guaranteed to be
correct, and just kind of a huge hack.

[Wez, 2011-02-01 16:23:09]

Patch set #4 provides the child-ID of the NPAPI plugin process to the host
renderer when it asks for a channel to the plugin.  The child-ID then replaces
the process-ID of the plugin that is normally supplied as part of resource
requests, which requires some changes to the WebKit "glue".

Having developed this patch, I think it's a(nother) gross hacky way to solve the
problem.

I think the Right Way is to supply the child-ID of _every_ child process of the
browser to it when it starts up, and update IPCs that pass process IDs to pass
child-IDs instead, or in addition.

[jam, 2011-02-01 18:20:30]

hi, sorry I just saw this.  I agree the latest patchset is too complicated.  Why
not just use the first patchset?

[wez1, 2011-02-01 19:27:15]

Patch Set 1 just fixes up Task Manager to assume that the supplied
origin_child_id is actually a process-ID, not a browser-allocated child-ID.

The problem with that is that in some cases origin_child_id *is* a child-ID
(specifically in ResourceDispatcherHost::BeginDownload and BeginSaveFile),
so different resource usage will get mis-matched.

At the moment the disparity just results in bogus Task Manager output but in
future it may lead to other problems if new code finds a use for the
originator details, so I thought I'd try mapping the plugin process-ID back
to a child-ID (Patch Set 2) and providing the child-ID in the request from
the renderer process (Patch Set 4).

The alternative would be to stick to always using process-IDs, which will
basically work, although the functions mentioned above would need some work,
and new code would need to take into account that process-IDs may be re-used
over time.

The other problem, which I addressed in a separate CL by adding an ugly
catch-all case to TaskManager::BytesRead(), is that some requests originate
outside ResourceDispatcherHost, and have neither an origin_child_id nor a
render_host_process_child_id associated with them (e.g. Omnibox quick search
requests).


On 1 February 2011 18:20, <jam@chromium.org> wrote:

> hi, sorry I just saw this.  I agree the latest patchset is too complicated.
>  Why
> not just use the first patchset?
>
>
> http://codereview.chromium.org/6328010/
>

[jam, 2011-02-01 19:59:10]

On 2011/02/01 19:27:15, wez_google.com wrote:
> Patch Set 1 just fixes up Task Manager to assume that the supplied
> origin_child_id is actually a process-ID, not a browser-allocated child-ID.
> 
> The problem with that is that in some cases origin_child_id *is* a child-ID
> (specifically in ResourceDispatcherHost::BeginDownload and BeginSaveFile),
> so different resource usage will get mis-matched.

I'm confused, you mean child_proc.handle()?

> 
> At the moment the disparity just results in bogus Task Manager output but in
> future it may lead to other problems if new code finds a use for the
> originator details, so I thought I'd try mapping the plugin process-ID back
> to a child-ID (Patch Set 2) and providing the child-ID in the request from
> the renderer process (Patch Set 4).
> 
> The alternative would be to stick to always using process-IDs, which will
> basically work, although the functions mentioned above would need some work,
> and new code would need to take into account that process-IDs may be re-used
> over time.

This is how the code originally used to be.  But it lead to race conditions when
process IDs are reused (leading to potential security issues).  We definitely
don't want to go back to that.

> 
> The other problem, which I addressed in a separate CL by adding an ugly
> catch-all case to TaskManager::BytesRead(), is that some requests originate
> outside ResourceDispatcherHost, and have neither an origin_child_id nor a
> render_host_process_child_id associated with them (e.g. Omnibox quick search
> requests).
> 
> On 1 February 2011 18:20, <mailto:jam@chromium.org> wrote:
> 
> > hi, sorry I just saw this.  I agree the latest patchset is too complicated.
> >  Why
> > not just use the first patchset?
> >
> >
> > http://codereview.chromium.org/6328010/
> >

[Wez, 2011-02-01 20:37:01]

On 1 February 2011 19:59, <jam@chromium.org> wrote:

> On 2011/02/01 19:27:15, wez_google.com wrote:
>
>> Patch Set 1 just fixes up Task Manager to assume that the supplied
>> origin_child_id is actually a process-ID, not a browser-allocated
>> child-ID.
>>
>
>  The problem with that is that in some cases origin_child_id *is* a
>> child-ID
>> (specifically in ResourceDispatcherHost::BeginDownload and BeginSaveFile),
>> so different resource usage will get mis-matched.
>>
>
> I'm confused, you mean child_proc.handle()?


I can't see a child_proc there but both of those functions have a "child_id"
parameter, which they supply to SetOriginProcessUniqueIDForRequest() to
attach to the underlying URLRequests as the "originator" of the request.  If
you trace things back then the one supplied to BeginDownload() appears to be
a PID but if you follow back far enough then both come from
RenderProcessHost::id() - the "child ID" the browser allocated unique to the
originating renderer child process.

(or were you asking something else?)


>  At the moment the disparity just results in bogus Task Manager output but
>> in
>> future it may lead to other problems if new code finds a use for the
>> originator details, so I thought I'd try mapping the plugin process-ID
>> back
>> to a child-ID (Patch Set 2) and providing the child-ID in the request from
>> the renderer process (Patch Set 4).
>>
>
>  The alternative would be to stick to always using process-IDs, which will
>> basically work, although the functions mentioned above would need some
>> work,
>> and new code would need to take into account that process-IDs may be
>> re-used
>> over time.
>>
>
> This is how the code originally used to be.  But it lead to race conditions
> when
> process IDs are reused (leading to potential security issues).  We
> definitely
> don't want to go back to that.


Ahhh, that explains a lot!

That's true for the render_process_host_child_id / child_id parameters that
get passed around, but it's less of a problem for the request originator
information, I think, since security is enforced based on which renderer a
request is *received* through, not which process it *originated* from.  So
it would be possible to replace origin_child_id with origin_pid and use that
for accounting in Task Manager, without changing the way child-IDs are used
elsewhere.

Mixing the two kinds of identifier makes this rather confusing, especially
if the parameter names are misleading.  Using browser-allocated IDs
throughout seems like the sanest approach, but that really requires that
child processes are told their ID when they start up, and that code is
sanitised not to pass values from GetProcId() by mistake.

[jam, 2011-02-01 23:14:30]

On Tue, Feb 1, 2011 at 12:36 PM, Wez <wez@chromium.org> wrote:

> On 1 February 2011 19:59, <jam@chromium.org> wrote:
>
>> On 2011/02/01 19:27:15, wez_google.com wrote:
>>
>>> Patch Set 1 just fixes up Task Manager to assume that the supplied
>>> origin_child_id is actually a process-ID, not a browser-allocated
>>> child-ID.
>>>
>>
>>  The problem with that is that in some cases origin_child_id *is* a
>>> child-ID
>>> (specifically in ResourceDispatcherHost::BeginDownload and
>>> BeginSaveFile),
>>> so different resource usage will get mis-matched.
>>>
>>
>> I'm confused, you mean child_proc.handle()?
>
>
> I can't see a child_proc there but both of those functions have a
> "child_id" parameter, which they supply
> to SetOriginProcessUniqueIDForRequest() to attach to the underlying
> URLRequests as the "originator" of the request.  If you trace things back
> then the one supplied to BeginDownload() appears to be a PID but if you
> follow back far enough then both come from RenderProcessHost::id() - the
> "child ID" the browser allocated unique to the originating renderer child
> process.
>
> (or were you asking something else?)
>

Since for plugin processes origin_child_id is correct,
in TaskManagerModel::OnBytesRead() we have a render process id, render view
id, and a child process id.  can't we just pass all three numbers to the UI
thread (right now we don't pass render view id), and then call
RenderViewHost::FromID() to get to the RenderProcessHost, and then check the
ids?  if it's not the same as origin_child_id, then we know it's a plugin
process and we can use it as a pid?

>
>
>>  At the moment the disparity just results in bogus Task Manager output but
>>> in
>>> future it may lead to other problems if new code finds a use for the
>>> originator details, so I thought I'd try mapping the plugin process-ID
>>> back
>>> to a child-ID (Patch Set 2) and providing the child-ID in the request
>>> from
>>> the renderer process (Patch Set 4).
>>>
>>
>>  The alternative would be to stick to always using process-IDs, which will
>>> basically work, although the functions mentioned above would need some
>>> work,
>>> and new code would need to take into account that process-IDs may be
>>> re-used
>>> over time.
>>>
>>
>> This is how the code originally used to be.  But it lead to race
>> conditions when
>> process IDs are reused (leading to potential security issues).  We
>> definitely
>> don't want to go back to that.
>
>
> Ahhh, that explains a lot!
>
> That's true for the render_process_host_child_id / child_id parameters that
> get passed around, but it's less of a problem for the request originator
> information, I think, since security is enforced based on which renderer a
> request is *received* through, not which process it *originated* from.  So
> it would be possible to replace origin_child_id with origin_pid and use that
> for accounting in Task Manager, without changing the way child-IDs are used
> elsewhere.
>
> Mixing the two kinds of identifier makes this rather confusing, especially
> if the parameter names are misleading.  Using browser-allocated IDs
> throughout seems like the sanest approach, but that really requires that
> child processes are told their ID when they start up, and that code is
> sanitised not to pass values from GetProcId() by mistake.
>

[brettw, 2011-02-01 23:24:13]

On Tue, Feb 1, 2011 at 3:14 PM, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Tue, Feb 1, 2011 at 12:36 PM, Wez <wez@chromium.org> wrote:
>>
>> On 1 February 2011 19:59, <jam@chromium.org> wrote:
>>>
>>> On 2011/02/01 19:27:15, wez_google.com wrote:
>>>>
>>>> Patch Set 1 just fixes up Task Manager to assume that the supplied
>>>> origin_child_id is actually a process-ID, not a browser-allocated
>>>> child-ID.
>>>
>>>> The problem with that is that in some cases origin_child_id *is* a
>>>> child-ID
>>>> (specifically in ResourceDispatcherHost::BeginDownload and
>>>> BeginSaveFile),
>>>> so different resource usage will get mis-matched.
>>>
>>> I'm confused, you mean child_proc.handle()?
>>
>> I can't see a child_proc there but both of those functions have a
>> "child_id" parameter, which they supply
>> to SetOriginProcessUniqueIDForRequest() to attach to the underlying
>> URLRequests as the "originator" of the request.  If you trace things back
>> then the one supplied to BeginDownload() appears to be a PID but if you
>> follow back far enough then both come from RenderProcessHost::id() - the
>> "child ID" the browser allocated unique to the originating renderer child
>> process.
>>
>> (or were you asking something else?)
>
> Since for plugin processes origin_child_id is correct,
> in TaskManagerModel::OnBytesRead() we have a render process id, render view
> id, and a child process id.  can't we just pass all three numbers to the UI
> thread (right now we don't pass render view id), and then call
> RenderViewHost::FromID() to get to the RenderProcessHost, and then check the
> ids?  if it's not the same as origin_child_id, then we know it's a plugin
> process and we can use it as a pid?

This approach seems kind of hacky and makes me sad. We had a previous
iteration that used an even more hacky version of this type of
checking which I really didn't like. But if this is the cleanest
solution without adding a whole bunch of ugly code, then I guess
that's OK.

Brett

[Wez, 2011-02-03 17:40:48]

Patch Set 5 implements the following approach:

1. Only set explicit (non-zero) originator ID for requests from plugins, etc
2. For those requests, always use a PID, not a child-ID

(1) could just as easily be "always use the renderer or browser PID as the
originator for requests", but the zero-PID approach avoided doing child ID to
PID lookups.

[jam, 2011-02-03 17:50:38]

On Thu, Feb 3, 2011 at 9:40 AM, <wez@chromium.org> wrote:

> Patch Set 5 implements the following approach:
>
> 1. Only set explicit (non-zero) originator ID for requests from plugins,
> etc
> 2. For those requests, always use a PID, not a child-ID
>
> (1) could just as easily be "always use the renderer or browser PID as the
> originator for requests", but the zero-PID approach avoided doing child ID
> to
> PID lookups.


It's better to optimize for code readability and maintainability, over
something that isn't a performance bottleneck.  i.e. if we can avoid special
cases, that's better.

>
>
>
> http://codereview.chromium.org/6328010/
>

[jam, 2011-02-03 22:58:07]

http://codereview.chromium.org/6328010/diff/18001/chrome/browser/net/url_requ...
File chrome/browser/net/url_request_tracking.h (right):

http://codereview.chromium.org/6328010/diff/18001/chrome/browser/net/url_requ...
chrome/browser/net/url_request_tracking.h:24: // interpreted as originating from
the Browser process.
I thought it's not also set for renderer processes?

http://codereview.chromium.org/6328010/diff/18001/chrome/browser/net/url_requ...
chrome/browser/net/url_request_tracking.h:25: void
SetOriginProcessIDForRequest(int id, net::URLRequest* request);
nit: can we use PID instead of ProcessID to further hint that it's not the
process child id?  i know the naming is confusing unfortunately

http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_dispatcher_host.cc:736:
chrome_browser_net::SetOriginProcessUniqueIDForRequest(child_id, request);
why take this out?

http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_dispatcher_host.cc:784:
chrome_browser_net::SetOriginProcessUniqueIDForRequest(child_id, request);
ditto

http://codereview.chromium.org/6328010/diff/18001/chrome/browser/task_manager...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

http://codereview.chromium.org/6328010/diff/18001/chrome/browser/task_manager...
chrome/browser/task_manager/task_manager_resource_providers.cc:243: // working
on the TabContent's behalf, so ignore it
I don't understand this line and below on 453.  what does returning NULL do? 
it's always preferable to treat all child processes the same way

nit: style guide is to have proper punctuation etc, so period at end.

http://codereview.chromium.org/6328010/diff/18001/chrome/common/child_process...
File chrome/common/child_process_info.h (right):

http://codereview.chromium.org/6328010/diff/18001/chrome/common/child_process...
chrome/common/child_process_info.h:72: int process_id() const { return
process_.pid(); }
nit: should be pid

[Wez, 2011-02-03 23:58:20]

On 3 February 2011 22:58, <jam@chromium.org> wrote:

>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/net/url_requ...
> chrome/browser/net/url_request_tracking.h:24: // interpreted as
> originating from the Browser process.
> I thought it's not also set for renderer processes?
>

You're quite right, that comment refers to the variant where the PID is set
even for renderer processes.  My mistake.


>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/net/url_requ...
> chrome/browser/net/url_request_tracking.h:25: void
> SetOriginProcessIDForRequest(int id, net::URLRequest* request);
> nit: can we use PID instead of ProcessID to further hint that it's not
> the process child id?  i know the naming is confusing unfortunately
>

Good idea.  Agreeing a similar abbreviation for child-ID (CID?) would also
help make comments more readable where code needs to work with both.


>
>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/resource_dispatcher_host.cc (left):
>
>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/resource_dispatcher_host.cc:736:
> chrome_browser_net::SetOriginProcessUniqueIDForRequest(child_id,
> request);
> why take this out?
>

The child_id values that BeginDownload() and BeginSaveFile() are supplied
are actual render process child IDs, not PIDs. We could look them up to
determine the PID to set, of course, but in this version of the patch PID==0
already means that where the request came from is identified by the renderer
and view IDs.


>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/resource_dispatcher_host.cc:784:
> chrome_browser_net::SetOriginProcessUniqueIDForRequest(child_id,
> request);
> ditto
>

"


>
>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/task_manager...
> File chrome/browser/task_manager/task_manager_resource_providers.cc
> (right):
>
>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/task_manager...
> chrome/browser/task_manager/task_manager_resource_providers.cc:243: //
> working on the TabContent's behalf, so ignore it
> I don't understand this line and below on 453.  what does returning NULL
> do?  it's always preferable to treat all child processes the same way
>

The original code determined the TabContents or BackgroundContents for the
specified renderer & routing IDs, then checked whether the renderer process
ID for the returned resource matched the origin PID.  If not then (as I
understand it) that means that the notification comes from a request made by
the TabContents or BackgroundContents on behalf of a plugin.  With the
change to have origin ID zero unless the request was made on behalf of a
process other than the renderer or browser, it's sufficient just to check
whether origin_id is non-zero.

The resource matching does look more involved than it needs to be, though.


>
> nit: style guide is to have proper punctuation etc, so period at end.
>

Will do - thanks.


>
>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/common/child_process...
> File chrome/common/child_process_info.h (right):
>
>
>
http://codereview.chromium.org/6328010/diff/18001/chrome/common/child_process...
> chrome/common/child_process_info.h:72: int process_id() const { return
> process_.pid(); }
> nit: should be pid


OK. :)

Wez

[awong, 2011-02-05 05:18:54]

Landed with minor fix in r73915

On 2011/02/03 23:58:20, Wez wrote:
> On 3 February 2011 22:58, <mailto:jam@chromium.org> wrote:
> 
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/net/url_requ...
> > chrome/browser/net/url_request_tracking.h:24: // interpreted as
> > originating from the Browser process.
> > I thought it's not also set for renderer processes?
> >
> 
> You're quite right, that comment refers to the variant where the PID is set
> even for renderer processes.  My mistake.
> 
> 
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/net/url_requ...
> > chrome/browser/net/url_request_tracking.h:25: void
> > SetOriginProcessIDForRequest(int id, net::URLRequest* request);
> > nit: can we use PID instead of ProcessID to further hint that it's not
> > the process child id?  i know the naming is confusing unfortunately
> >
> 
> Good idea.  Agreeing a similar abbreviation for child-ID (CID?) would also
> help make comments more readable where code needs to work with both.
> 
> 
> >
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
> > File chrome/browser/renderer_host/resource_dispatcher_host.cc (left):
> >
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
> > chrome/browser/renderer_host/resource_dispatcher_host.cc:736:
> > chrome_browser_net::SetOriginProcessUniqueIDForRequest(child_id,
> > request);
> > why take this out?
> >
> 
> The child_id values that BeginDownload() and BeginSaveFile() are supplied
> are actual render process child IDs, not PIDs. We could look them up to
> determine the PID to set, of course, but in this version of the patch PID==0
> already means that where the request came from is identified by the renderer
> and view IDs.
> 
> 
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/renderer_hos...
> > chrome/browser/renderer_host/resource_dispatcher_host.cc:784:
> > chrome_browser_net::SetOriginProcessUniqueIDForRequest(child_id,
> > request);
> > ditto
> >
> 
> "
> 
> 
> >
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/task_manager...
> > File chrome/browser/task_manager/task_manager_resource_providers.cc
> > (right):
> >
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/browser/task_manager...
> > chrome/browser/task_manager/task_manager_resource_providers.cc:243: //
> > working on the TabContent's behalf, so ignore it
> > I don't understand this line and below on 453.  what does returning NULL
> > do?  it's always preferable to treat all child processes the same way
> >
> 
> The original code determined the TabContents or BackgroundContents for the
> specified renderer & routing IDs, then checked whether the renderer process
> ID for the returned resource matched the origin PID.  If not then (as I
> understand it) that means that the notification comes from a request made by
> the TabContents or BackgroundContents on behalf of a plugin.  With the
> change to have origin ID zero unless the request was made on behalf of a
> process other than the renderer or browser, it's sufficient just to check
> whether origin_id is non-zero.
> 
> The resource matching does look more involved than it needs to be, though.
> 
> 
> >
> > nit: style guide is to have proper punctuation etc, so period at end.
> >
> 
> Will do - thanks.
> 
> 
> >
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/common/child_process...
> > File chrome/common/child_process_info.h (right):
> >
> >
> >
>
http://codereview.chromium.org/6328010/diff/18001/chrome/common/child_process...
> > chrome/common/child_process_info.h:72: int process_id() const { return
> > process_.pid(); }
> > nit: should be pid
> 
> 
> OK. :)
> 
> Wez