Several Site Details / Memory metrics fixes

chrome/browser/memory_details.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/memory_details.h"
 
 #include <algorithm>
 #include <set>
 
 #include "base/bind.h"
@@ skipping 197 matching lines @@
 }
 
 void MemoryDetails::CollectChildInfoOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
   const pid_t zygote_pid = content::ZygoteHost::GetInstance()->GetPid();
 #endif
 
   ProcessData* const chrome_browser = ChromeBrowser();
+
+  // First pass, collate the widgets by process ID.
+  std::map<base::ProcessId, std::vector<RenderWidgetHost*>> widgets_by_pid;
+  scoped_ptr<content::RenderWidgetHostIterator> widget_it(
+      RenderWidgetHost::GetRenderWidgetHosts());
+  while (content::RenderWidgetHost* widget = widget_it->GetNextHost()) {
+    // Ignore processes that don't have a connection, such as crashed tabs.
+    if (!widget->GetProcess()->HasConnection())
+      continue;
+    base::ProcessId pid = base::GetProcId(widget->GetProcess()->GetHandle());
+    widgets_by_pid[pid].push_back(widget);
+  }
+
   // Get more information about the process.
-  for (size_t index = 0; index < chrome_browser->processes.size();
-      index++) {
-    // Check if it's a renderer, if so get the list of page titles in it and
-    // check if it's a diagnostics-related process.  We skip about:memory pages.
-    // Iterate the RenderProcessHosts to find the tab contents.
-    ProcessMemoryInformation& process =
-        chrome_browser->processes[index];
+  for (ProcessMemoryInformation& process : chrome_browser->processes) {
+    // If there's at least one widget in the process, it is some kind of
+    // renderer process belonging to this browser. All these widgets will share
+    // a RenderProcessHost.
+    content::RenderProcessHost* render_process_host = nullptr;
+    if (!widgets_by_pid[process.pid].empty()) {
+      // Mark it as a normal renderer process, if we don't refine it to some
+      // other |renderer_type| later.
+      process.process_type = content::PROCESS_TYPE_RENDERER;
+      process.renderer_type = ProcessMemoryInformation::RENDERER_NORMAL;
+      render_process_host = widgets_by_pid[process.pid].front()->GetProcess();
+    }
 
-    scoped_ptr<content::RenderWidgetHostIterator> widgets(
-        RenderWidgetHost::GetRenderWidgetHosts());
-    while (content::RenderWidgetHost* widget = widgets->GetNextHost()) {
-      content::RenderProcessHost* render_process_host =
-          widget->GetProcess();
-      DCHECK(render_process_host);
-      // Ignore processes that don't have a connection, such as crashed tabs.
-      if (!render_process_host->HasConnection() ||
-          process.pid != base::GetProcId(render_process_host->GetHandle())) {
-        continue;
-      }
-
-      // The RenderProcessHost may host multiple WebContentses.  Any
-      // of them which contain diagnostics information make the whole
-      // process be considered a diagnostics process.
-      if (!widget->IsRenderView())
-        continue;
-
-      process.process_type = content::PROCESS_TYPE_RENDERER;
-      bool is_extension = false;
-      RenderViewHost* host = RenderViewHost::From(widget);
 #if defined(ENABLE_EXTENSIONS)
+    // Determine if this is an extension process.
+    bool process_is_for_extensions = false;
+    if (render_process_host) {
       content::BrowserContext* context =
           render_process_host->GetBrowserContext();
       extensions::ExtensionRegistry* extension_registry =
           extensions::ExtensionRegistry::Get(context);
       extensions::ProcessMap* extension_process_map =
           extensions::ProcessMap::Get(context);
-      is_extension = extension_process_map->Contains(
-          host->GetProcess()->GetID());
+      process_is_for_extensions =
+          extension_process_map->Contains(render_process_host->GetID());
+
+      // For our purposes, don't count processes containing only hosted apps
+      // as extension processes. See also: crbug.com/102533.
+      std::set<std::string> extension_ids =
+          extension_process_map->GetExtensionsInProcess(
+              render_process_host->GetID());
+      for (std::set<std::string>::iterator iter = extension_ids.begin();
+           iter != extension_ids.end(); ++iter) {
+        const Extension* extension =
+            extension_registry->enabled_extensions().GetByID(*iter);
+        if (extension && !extension->is_hosted_app()) {
+          process.renderer_type = ProcessMemoryInformation::RENDERER_EXTENSION;
+          break;
+        }
+      }
+    }
 #endif
 
-      WebContents* contents = WebContents::FromRenderViewHost(host);
-      GURL url;
-      if (contents) {
-        url = contents->GetURL();
-        SiteData* site_data =
-            &chrome_browser->site_data[contents->GetBrowserContext()];
-        SiteDetails::CollectSiteInfo(contents, site_data);
+    // Use the list of widgets to iterate over the WebContents instances whose
+    // main RenderFrameHosts are in |process|. Refine our determination of the
+    // |process.renderer_type|, and record the page titles.
+    for (content::RenderWidgetHost* widget : widgets_by_pid[process.pid]) {
+      DCHECK_EQ(render_process_host, widget->GetProcess());
+
+      if (!widget->IsRenderView())
+        continue;
+
+      RenderViewHost* rvh = RenderViewHost::From(widget);
+      WebContents* contents = WebContents::FromRenderViewHost(rvh);
+
+      // Assume that an RVH without a web contents is an interstitial.
+      if (!contents) {
+        process.renderer_type = ProcessMemoryInformation::RENDERER_INTERSTITIAL;
+        continue;
       }
+
+      // If this is a RVH for a subframe; skip it to avoid double-counting the
+      // WebContents.
+      if (rvh != contents->GetRenderViewHost())
+        continue;
+
+      // The rest of this block will happen only once per WebContents.
+      GURL page_url = contents->GetURL();

[Charlie Reis, 2015/10/23 19:03:40]

Using the deprecated GetURL method was a bug, since that returns the visible URL
(which might be pending, and thus not correspond to GetRenderViewHost).  We
should use GetLastCommittedURL instead.

(I'm fine with fixing that in either this CL or a separate CL, depending on how
much behavior change you want in this one.)

[ncarter (slow), 2015/10/29 19:55:06]

Done.

+      SiteData* site_data =
+          &chrome_browser->site_data[contents->GetBrowserContext()];
+      SiteDetails::CollectSiteInfo(contents, site_data);
+
+      bool is_webui =
+          rvh->GetEnabledBindings() & content::BINDINGS_POLICY_WEB_UI;
+
+      if (is_webui) {
+        process.renderer_type = ProcessMemoryInformation::RENDERER_CHROME;
+      }
+
 #if defined(ENABLE_EXTENSIONS)
-      extensions::ViewType type = extensions::GetViewType(contents);
-#endif
-      if (host->GetEnabledBindings() & content::BINDINGS_POLICY_WEB_UI) {
-        process.renderer_type = ProcessMemoryInformation::RENDERER_CHROME;
-      } else if (is_extension) {
-#if defined(ENABLE_EXTENSIONS)
-        // For our purposes, don't count processes containing only hosted apps
-        // as extension processes. See also: crbug.com/102533.
-        std::set<std::string> extension_ids =
-            extension_process_map->GetExtensionsInProcess(
-            host->GetProcess()->GetID());
-        for (std::set<std::string>::iterator iter = extension_ids.begin();
-             iter != extension_ids.end(); ++iter) {
-          const Extension* extension =
-              extension_registry->enabled_extensions().GetByID(*iter);
-          if (extension && !extension->is_hosted_app()) {
-            process.renderer_type =
-                ProcessMemoryInformation::RENDERER_EXTENSION;
-            break;
-          }
-        }
-#endif
-      }
-#if defined(ENABLE_EXTENSIONS)
-      if (is_extension) {
+      if (!is_webui && process_is_for_extensions) {
         const Extension* extension =
-            extension_registry->enabled_extensions().GetByID(url.host());
+            extensions::ExtensionRegistry::Get(
+                render_process_host->GetBrowserContext())
+                ->enabled_extensions()
+                .GetByID(page_url.host());
         if (extension) {
           base::string16 title = base::UTF8ToUTF16(extension->name());
           process.titles.push_back(title);
           process.renderer_type =
               ProcessMemoryInformation::RENDERER_EXTENSION;
           continue;
         }
       }
-#endif
 
-      if (!contents) {
-        process.renderer_type =
-            ProcessMemoryInformation::RENDERER_INTERSTITIAL;
-        continue;
-      }
-
-#if defined(ENABLE_EXTENSIONS)
+      extensions::ViewType type = extensions::GetViewType(contents);
       if (type == extensions::VIEW_TYPE_BACKGROUND_CONTENTS) {
-        process.titles.push_back(base::UTF8ToUTF16(url.spec()));
+        process.titles.push_back(base::UTF8ToUTF16(page_url.spec()));
         process.renderer_type =
             ProcessMemoryInformation::RENDERER_BACKGROUND_APP;
         continue;
       }
 #endif
 
-      // Since we have a WebContents and and the renderer type hasn't been
-      // set yet, it must be a normal tabbed renderer.
-      if (process.renderer_type == ProcessMemoryInformation::RENDERER_UNKNOWN)
-        process.renderer_type = ProcessMemoryInformation::RENDERER_NORMAL;
-
       base::string16 title = contents->GetTitle();
       if (!title.length())
         title = l10n_util::GetStringUTF16(IDS_DEFAULT_TAB_TITLE);
       process.titles.push_back(title);
 
+      // The presence of a single WebContents with a diagnostics page will make
+      // make the whole process be considered a diagnostics process.

[Charlie Reis, 2015/10/23 19:03:39]

Thank you for moving this comment!  It made no sense where it was before.

[ncarter (slow), 2015/10/29 19:55:06]

Done.

+      //
       // We need to check the pending entry as well as the virtual_url to
       // see if it's a chrome://memory URL (we don't want to count these in
       // the total memory usage of the browser).
       //
       // When we reach here, chrome://memory will be the pending entry since
       // we haven't responded with any data such that it would be committed.
       // If you have another chrome://memory tab open (which would be
       // committed), we don't want to count it either, so we also check the
       // last committed entry.
       //
@@ skipping 15 matching lines @@
     }
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
     if (process.pid == zygote_pid) {
       process.process_type = content::PROCESS_TYPE_ZYGOTE;
     }
 #endif
   }
 
   // Get rid of other Chrome processes that are from a different profile.
-  for (size_t index = 0; index < chrome_browser->processes.size();
-      index++) {
-    if (chrome_browser->processes[index].process_type ==
-        content::PROCESS_TYPE_UNKNOWN) {
-      chrome_browser->processes.erase(
-          chrome_browser->processes.begin() + index);
-      index--;
-    }
-  }
+  auto is_unknown = [](ProcessMemoryInformation& process) {
+    return process.process_type == content::PROCESS_TYPE_UNKNOWN;
+  };
+  auto& vector = chrome_browser->processes;
+  vector.erase(std::remove_if(vector.begin(), vector.end(), is_unknown),
+               vector.end());
 
   OnDetailsAvailable();
 }