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.

Review URL: http://codereview.chromium.org/6328010
Patch from James Weatherall <wez@chromium.org>.

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@73884 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 69 insertions, 87 deletions

diff --git a/chrome/browser/net/url_request_tracking.cc b/chrome/browser/net/url_request_tracking.cc
index a931856..020aad1 100644
--- a/chrome/browser/net/url_request_tracking.cc
+++ b/chrome/browser/net/url_request_tracking.cc
@@ -11,37 +11,37 @@ namespace {
 
 // The value is not important, this address is used as the unique key for the
 // PID.
-const void* kOriginProcessUniqueIDKey = 0;
+const void* kOriginPidKey = 0;
 
-class UniqueIDData : public net::URLRequest::UserData {
+class OriginPidData : public net::URLRequest::UserData {
  public:
-  explicit UniqueIDData(int id) : id_(id) {}
-  virtual ~UniqueIDData() {}
+  explicit OriginPidData(int pid) : pid_(pid) {}
+  virtual ~OriginPidData() {}
 
-  int id() const { return id_; }
-  void set_id(int id) { id_ = id; }
+  int pid() const { return pid_; }
+  void set_pid(int pid) { pid_ = pid; }
 
  private:
-  int id_;
+  int pid_;
 
-  DISALLOW_COPY_AND_ASSIGN(UniqueIDData);
+  DISALLOW_COPY_AND_ASSIGN(OriginPidData);
 };
 
 }  // namespace
 
 namespace chrome_browser_net {
 
-void SetOriginProcessUniqueIDForRequest(int id, net::URLRequest* request) {
+void SetOriginPIDForRequest(int pid, net::URLRequest* request) {
   // The request will take ownership.
-  request->SetUserData(&kOriginProcessUniqueIDKey, new UniqueIDData(id));
+  request->SetUserData(&kOriginPidKey, new OriginPidData(pid));
 }
 
-int GetOriginProcessUniqueIDForRequest(const net::URLRequest* request) {
-  const UniqueIDData* data = static_cast<const UniqueIDData*>(
-      request->GetUserData(&kOriginProcessUniqueIDKey));
+int GetOriginPIDForRequest(const net::URLRequest* request) {
+  const OriginPidData* data = static_cast<const OriginPidData*>(
+      request->GetUserData(&kOriginPidKey));
   if (!data)
-    return -1;
-  return data->id();
+    return 0;
+  return data->pid();
 }
 
 }  // namespace chrome_browser_net
diff --git a/chrome/browser/net/url_request_tracking.h b/chrome/browser/net/url_request_tracking.h
index 0e7a7156..497dfb2 100644
--- a/chrome/browser/net/url_request_tracking.h
+++ b/chrome/browser/net/url_request_tracking.h
@@ -18,20 +18,19 @@ namespace chrome_browser_net {
 // place allows us to do more general things, such as assigning traffic for the
 // network view in the task manager.
 //
-// If you make a request on behalf of a child process, please call this
-// function. The default value will be -1 which will be interprepreted as
-// originating from the browser itself.
+// If you make a request on behalf of a child process other than a renderer,
+// please call this function to store its PID (NOT its browser-assigned unique
+// child ID).  For requests originating in a renderer or the browser itself,
+// set a PID of zero (the default).
 //
-// The ID is the child process' unique ID (not a PID) of the process originating
-// the request. This is normally the renderer corresponding to the load. If a
-// plugin process does a request through a renderer process this will be the
-// plugin (the originator of the request).
-void SetOriginProcessUniqueIDForRequest(int id, net::URLRequest* request);
+// TODO(wez): Get rid of the zero-PID hack & enforce that one is always set.
+void SetOriginPIDForRequest(int pid, net::URLRequest* request);
 
-// Returns the child process' unique ID that has been previously set by
-// SetOriginProcessUniqueIDForRequest. If no ID has been set, the return
-// value is -1. We use this to identify requests made by the browser process.
-int GetOriginProcessUniqueIDForRequest(const net::URLRequest* request);
+// Returns the process ID of the request's originator, previously stored with
+// SetOriginProcessIDForRequest, or zero if no PID has been set.  A PID of zero
+// should be interpreted as meaning the request originated from a renderer
+// process, or within the browser itself.
+int GetOriginPIDForRequest(const net::URLRequest* request);
 
 }  // namespace chrome_browser_net
 
diff --git a/chrome/browser/renderer_host/resource_dispatcher_host.cc b/chrome/browser/renderer_host/resource_dispatcher_host.cc
index 50c3a81..5bfe445 100644
--- a/chrome/browser/renderer_host/resource_dispatcher_host.cc
+++ b/chrome/browser/renderer_host/resource_dispatcher_host.cc
@@ -199,13 +199,13 @@ std::vector<int> GetAllNetErrorCodes() {
 }
 
 #if defined(OS_WIN)
-#pragma warning (disable: 4748)
-#pragma optimize( "", off )
+#pragma warning(disable: 4748)
+#pragma optimize("", off)
 #endif
 
 #if defined(OS_WIN)
-#pragma optimize( "", on )
-#pragma warning (default: 4748)
+#pragma optimize("", on)
+#pragma warning(default: 4748)
 #endif
 
 }  // namespace
@@ -521,8 +521,8 @@ void ResourceDispatcherHost::BeginRequest(
   ApplyExtensionLocalizationFilter(request_data.url, request_data.resource_type,
                                    extra_info);
   SetRequestInfo(request, extra_info);  // Request takes ownership.
-  chrome_browser_net::SetOriginProcessUniqueIDForRequest(
-      request_data.origin_child_id, request);
+  chrome_browser_net::SetOriginPIDForRequest(
+      request_data.origin_pid, request);
 
   if (request->url().SchemeIs(chrome::kBlobScheme) && context) {
     // Hang on to a reference to ensure the blob is not released prior
@@ -736,7 +736,6 @@ void ResourceDispatcherHost::BeginDownload(
   ResourceDispatcherHostRequestInfo* extra_info =
       CreateRequestInfoForBrowserRequest(handler, child_id, route_id, true);
   SetRequestInfo(request, extra_info);  // Request takes ownership.
-  chrome_browser_net::SetOriginProcessUniqueIDForRequest(child_id, request);
 
   BeginRequestInternal(request);
 }
@@ -784,7 +783,6 @@ void ResourceDispatcherHost::BeginSaveFile(
   ResourceDispatcherHostRequestInfo* extra_info =
       CreateRequestInfoForBrowserRequest(handler, child_id, route_id, false);
   SetRequestInfo(request, extra_info);  // Request takes ownership.
-  chrome_browser_net::SetOriginProcessUniqueIDForRequest(child_id, request);
 
   BeginRequestInternal(request);
 }
diff --git a/chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc b/chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc
index a619355..bc3ab35 100644
--- a/chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc
+++ b/chrome/browser/renderer_host/resource_dispatcher_host_unittest.cc
@@ -73,7 +73,7 @@ static ViewHostMsg_Resource_Request CreateResourceRequest(
   request.frame_origin = "null";
   request.main_frame_origin = "null";
   request.load_flags = 0;
-  request.origin_child_id = 0;
+  request.origin_pid = 0;
   request.resource_type = type;
   request.request_context = 0;
   request.appcache_host_id = appcache::kNoHostId;
diff --git a/chrome/browser/task_manager/task_manager.cc b/chrome/browser/task_manager/task_manager.cc
index 4c3634f..1f113b7 100644
--- a/chrome/browser/task_manager/task_manager.cc
+++ b/chrome/browser/task_manager/task_manager.cc
@@ -839,7 +839,7 @@ void TaskManagerModel::BytesRead(BytesReadParam param) {
   TaskManager::Resource* resource = NULL;
   for (ResourceProviderList::iterator iter = providers_.begin();
        iter != providers_.end(); ++iter) {
-    resource = (*iter)->GetResource(param.origin_child_id,
+    resource = (*iter)->GetResource(param.origin_pid,
                                     param.render_process_host_child_id,
                                     param.routing_id);
     if (resource)
@@ -851,9 +851,8 @@ void TaskManagerModel::BytesRead(BytesReadParam param) {
     // tab that started a download was closed, or the request may have had
     // no originating resource associated with it in the first place.
     // We attribute orphaned/unaccounted activity to the Browser process.
-    int browser_pid = base::GetCurrentProcId();
-    CHECK(param.origin_child_id != browser_pid);
-    param.origin_child_id = browser_pid;
+    CHECK(param.origin_pid || (param.render_process_host_child_id != -1));
+    param.origin_pid = 0;
     param.render_process_host_child_id = param.routing_id = -1;
     BytesRead(param);
     return;
@@ -904,15 +903,18 @@ void TaskManagerModel::OnBytesRead(net::URLRequestJob* job, const char* buf,
                                                &render_process_host_child_id,
                                                &routing_id);
 
+  // Get the origin PID of the request's originator.  This will only be set for
+  // plugins - for renderer or browser initiated requests it will be zero.
+  int origin_pid =
+      chrome_browser_net::GetOriginPIDForRequest(job->request());
+
   // This happens in the IO thread, post it to the UI thread.
-  int origin_child_id =
-      chrome_browser_net::GetOriginProcessUniqueIDForRequest(job->request());
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(
           this,
           &TaskManagerModel::BytesRead,
-          BytesReadParam(origin_child_id,
+          BytesReadParam(origin_pid,
           render_process_host_child_id,
           routing_id, byte_count)));
 }
diff --git a/chrome/browser/task_manager/task_manager.h b/chrome/browser/task_manager/task_manager.h
index d71106b..1f606de 100644
--- a/chrome/browser/task_manager/task_manager.h
+++ b/chrome/browser/task_manager/task_manager.h
@@ -345,21 +345,20 @@ class TaskManagerModel : public net::URLRequestJobTracker::JobObserver,
 
   // This struct is used to exchange information between the io and ui threads.
   struct BytesReadParam {
-    BytesReadParam(int origin_child_id,
+    BytesReadParam(int origin_pid,
                    int render_process_host_child_id,
                    int routing_id,
                    int byte_count)
-        : origin_child_id(origin_child_id),
+        : origin_pid(origin_pid),
           render_process_host_child_id(render_process_host_child_id),
           routing_id(routing_id),
           byte_count(byte_count) {}
 
-    // This is the child ID of the originator of the request. It will often be
-    // the same as the render_process_host_child_id, but will be different when
-    // another sub-process like a plugin is routing requests through a renderer.
-    int origin_child_id;
+    // The process ID that triggered the request.  For plugin requests this
+    // will differ from the renderer process ID.
+    int origin_pid;
 
-    // The child ID of the RenderProcessHist this request was routed through.
+    // The child ID of the RenderProcessHost this request was routed through.
     int render_process_host_child_id;
 
     int routing_id;
diff --git a/chrome/browser/task_manager/task_manager_resource_providers.cc b/chrome/browser/task_manager/task_manager_resource_providers.cc
index 6bc429f..18d41f4 100644
--- a/chrome/browser/task_manager/task_manager_resource_providers.cc
+++ b/chrome/browser/task_manager/task_manager_resource_providers.cc
@@ -234,23 +234,14 @@ TaskManager::Resource* TaskManagerTabContentsResourceProvider::GetResource(
     int origin_pid,
     int render_process_host_id,
     int routing_id) {
-
   TabContents* tab_contents =
       tab_util::GetTabContentsByID(render_process_host_id, routing_id);
   if (!tab_contents)  // Not one of our resource.
     return NULL;
 
-  base::ProcessHandle process_handle =
-      tab_contents->GetRenderProcessHost()->GetHandle();
-  if (!process_handle) {
-    // We should not be holding on to a dead tab (it should have been removed
-    // through the NOTIFY_TAB_CONTENTS_DISCONNECTED notification.
-    NOTREACHED();
-    return NULL;
-  }
-
-  int pid = base::GetProcId(process_handle);
-  if (pid != origin_pid)
+  // If an origin PID was specified then the request originated in a plugin
+  // working on the TabContent's behalf, so ignore it.
+  if (origin_pid)
     return NULL;
 
   std::map<TabContents*, TaskManagerTabContentsResource*>::iterator
@@ -453,19 +444,14 @@ TaskManagerBackgroundContentsResourceProvider::GetResource(
     int origin_pid,
     int render_process_host_id,
     int routing_id) {
-
   BackgroundContents* contents = BackgroundContents::GetBackgroundContentsByID(
       render_process_host_id, routing_id);
   if (!contents)  // This resource no longer exists.
     return NULL;
 
-  base::ProcessHandle process_handle =
-      contents->render_view_host()->process()->GetHandle();
-  if (!process_handle) // Process crashed.
-    return NULL;
-
-  int pid = base::GetProcId(process_handle);
-  if (pid != origin_pid)
+  // If an origin PID was specified, the request is from a plugin, not the
+  // render view host process
+  if (origin_pid)
     return NULL;
 
   std::map<BackgroundContents*,
@@ -642,7 +628,7 @@ TaskManagerChildProcessResource::TaskManagerChildProcessResource(
       network_usage_support_(false) {
   // We cache the process id because it's not cheap to calculate, and it won't
   // be available when we get the plugin disconnected notification.
-  pid_ = child_proc.id();
+  pid_ = child_proc.pid();
   if (!default_icon_) {
     ResourceBundle& rb = ResourceBundle::GetSharedInstance();
     default_icon_ = rb.GetBitmapNamed(IDR_PLUGIN);
@@ -1220,8 +1206,8 @@ SkBitmap* TaskManagerBrowserProcessResource::default_icon_ = NULL;
 
 TaskManagerBrowserProcessResource::TaskManagerBrowserProcessResource()
     : title_() {
-  pid_ = base::GetCurrentProcId();
-  bool success = base::OpenPrivilegedProcessHandle(pid_, &process_);
+  int pid = base::GetCurrentProcId();
+  bool success = base::OpenPrivilegedProcessHandle(pid, &process_);
   DCHECK(success);
 #if defined(OS_WIN)
   if (!default_icon_) {
@@ -1312,7 +1298,7 @@ TaskManager::Resource* TaskManagerBrowserProcessResourceProvider::GetResource(
     int origin_pid,
     int render_process_host_id,
     int routing_id) {
-  if (origin_pid != resource_.process_id()) {
+  if (origin_pid || render_process_host_id != -1) {
     return NULL;
   }
 
diff --git a/chrome/browser/task_manager/task_manager_resource_providers.h b/chrome/browser/task_manager/task_manager_resource_providers.h
index bfa13a0..d515c57 100644
--- a/chrome/browser/task_manager/task_manager_resource_providers.h
+++ b/chrome/browser/task_manager/task_manager_resource_providers.h
@@ -220,12 +220,8 @@ class TaskManagerChildProcessResource : public TaskManager::Resource {
   virtual bool SupportNetworkUsage() const OVERRIDE;
   virtual void SetSupportNetworkUsage() OVERRIDE;
 
-  // Returns the pid of the child process.
-  int process_id() const { return pid_; }
-
  private:
   ChildProcessInfo child_process_;
-  int pid_;
   mutable string16 title_;
   bool network_usage_support_;
 
diff --git a/chrome/common/child_process_info.h b/chrome/common/child_process_info.h
index 4d5b721..b083ea5 100644
--- a/chrome/common/child_process_info.h
+++ b/chrome/common/child_process_info.h
@@ -68,6 +68,9 @@ class ChildProcessInfo {
   // Getter to the process handle.
   base::ProcessHandle handle() const { return process_.handle(); }
 
+  // Getter to the process ID.
+  int pid() const { return process_.pid(); }
+
   // The unique identifier for this child process. This identifier is NOT a
   // process ID, and will be unique for all types of child process for
   // one run of the browser.
diff --git a/chrome/common/render_messages_params.cc b/chrome/common/render_messages_params.cc
index 59a17bd..3362704 100644
--- a/chrome/common/render_messages_params.cc
+++ b/chrome/common/render_messages_params.cc
@@ -61,7 +61,7 @@ ViewMsg_ClosePage_Params::~ViewMsg_ClosePage_Params() {
 
 ViewHostMsg_Resource_Request::ViewHostMsg_Resource_Request()
     : load_flags(0),
-      origin_child_id(0),
+      origin_pid(0),
       resource_type(ResourceType::MAIN_FRAME),
       request_context(0),
       appcache_host_id(0),
@@ -924,7 +924,7 @@ void ParamTraits<ViewHostMsg_Resource_Request>::Write(Message* m,
   WriteParam(m, p.main_frame_origin);
   WriteParam(m, p.headers);
   WriteParam(m, p.load_flags);
-  WriteParam(m, p.origin_child_id);
+  WriteParam(m, p.origin_pid);
   WriteParam(m, p.resource_type);
   WriteParam(m, p.request_context);
   WriteParam(m, p.appcache_host_id);
@@ -947,7 +947,7 @@ bool ParamTraits<ViewHostMsg_Resource_Request>::Read(const Message* m,
       ReadParam(m, iter, &r->main_frame_origin) &&
       ReadParam(m, iter, &r->headers) &&
       ReadParam(m, iter, &r->load_flags) &&
-      ReadParam(m, iter, &r->origin_child_id) &&
+      ReadParam(m, iter, &r->origin_pid) &&
       ReadParam(m, iter, &r->resource_type) &&
       ReadParam(m, iter, &r->request_context) &&
       ReadParam(m, iter, &r->appcache_host_id) &&
@@ -973,7 +973,7 @@ void ParamTraits<ViewHostMsg_Resource_Request>::Log(const param_type& p,
   l->append(", ");
   LogParam(p.load_flags, l);
   l->append(", ");
-  LogParam(p.origin_child_id, l);
+  LogParam(p.origin_pid, l);
   l->append(", ");
   LogParam(p.resource_type, l);
   l->append(", ");
diff --git a/chrome/common/render_messages_params.h b/chrome/common/render_messages_params.h
index ce4b18c..f44091c 100644
--- a/chrome/common/render_messages_params.h
+++ b/chrome/common/render_messages_params.h
@@ -456,10 +456,9 @@ struct ViewHostMsg_Resource_Request {
   // net::URLRequest load flags (0 by default).
   int load_flags;
 
-  // Unique ID of process that originated this request. For normal renderer
-  // requests, this will be the ID of the renderer. For plugin requests routed
-  // through the renderer, this will be the plugin's ID.
-  int origin_child_id;
+  // Process ID from which this request originated, or zero if it originated
+  // in the renderer itself.
+  int origin_pid;
 
   // What this resource load is for (main frame, sub-frame, sub-resource,
   // object).
diff --git a/chrome/common/resource_dispatcher.cc b/chrome/common/resource_dispatcher.cc
index e59a464..86321b5 100644
--- a/chrome/common/resource_dispatcher.cc
+++ b/chrome/common/resource_dispatcher.cc
@@ -106,7 +106,7 @@ IPCResourceLoaderBridge::IPCResourceLoaderBridge(
   request_.main_frame_origin = request_info.main_frame_origin;
   request_.headers = request_info.headers;
   request_.load_flags = request_info.load_flags;
-  request_.origin_child_id = request_info.requestor_pid;
+  request_.origin_pid = request_info.requestor_pid;
   request_.resource_type = request_info.request_type;
   request_.request_context = request_info.request_context;
   request_.appcache_host_id = request_info.appcache_host_id;