Several Site Details / Memory metrics fixes

Several Site Details / Memory metrics fixes

 - Fix possible O(n^4) worst case behavior (processes x widgets x
   frames x siteinstances) for stats gathering.

 - Show subframe-only processes in about:memory, which also causes them to
   be reported in SiteIsolation.CurrentRendererProcessCount.

 - Because SiteIsolation.CurrentRendererProcessCount is fixed, we need
   a new family of metrics, SiteIsolation.IsolateNothing*, which simulates
   the process-per-siteinstance process model. In tests, IsolateNothing
   is expected to have the value that CurrentRenderProcessCount used to.

 - Implement a more correct treatment of site isolation stats, which looks at
   parent/child transitions and handles ProcessPerSite properly. This is
   required for IsolateNothing to work correctly. Since options for
   walking the frame tree outside of content are pretty limited, this uses
   WebContents::ForEachFrame and (!) memoization.

 - Eliminate WebContents::GetSitesInTab; move its about:blank logic into
   the new frametree walking code in SiteDetails, which was the only caller.

BUG=454526, 542909
TEST=browser_tests

Committed: https://crrev.com/b2545f729554c7cbc682504d08ae450155953985
Cr-Commit-Position: refs/heads/master@{#357178}

[ncarter (slow), 2015-10-16 04:51:19]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 04:51:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406133002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406133002/20001

[commit-bot: I haz the power, 2015-10-16 06:13:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-16 06:13:54]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2015-10-16 17:13:51]

nick@chromium.org changed reviewers:
+ creis@chromium.org

[ncarter (slow), 2015-10-16 17:13:51]

Hi Charlie,

Please review this change. Not urgent, but it'll be nice to have.

[Charlie Reis, 2015-10-22 23:52:44]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2015-10-22 23:52:44]

Apologies that I haven't been able to get to this sooner.  Nasko, any chance
you'd want to review this, since you're blocked on it and will be editing this
code as well in https://crbug.com/542921?  If you're overloaded as well, I can
try to make time tomorrow.

nit: Add 542909 to the bug list in the CL description.

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

Great.  One bug and a few nits below, but otherwise LGTM!  Should be much
faster, and good to have the explicit isolate nothing scenario (and fix for
about:memory).  Thanks!

Reminder: list 542909 in the CL description.

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/memory_d...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/memory_d...
chrome/browser/memory_details.cc:299: GURL page_url = contents->GetURL();
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.)

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/memory_d...
chrome/browser/memory_details.cc:342: // make the whole process be considered a
diagnostics process.
Thank you for moving this comment!  It made no sense where it was before.

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/site_det...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/site_det...
chrome/browser/site_details.cc:59: void
CollectForScenario(std::map<RenderFrameHost*, GURL>* memo,
nit: memo -> frame_urls?

We could add a comment above saying something like:
Update |scenario|'s data for the given frame.  Memoize each frame's computed URL
in |frame_urls| so it can be reused when visiting its children.

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/site_det...
chrome/browser/site_details.cc:78: if (frame->GetParent()) {
// Treat a subframe as part of its parent site if neither needs isolation.

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/site_det...
chrome/browser/site_details.cc:88: // If we don't need a dedicated process, and
aren't living it a process-
nit: s/it/in/

[ncarter (slow), 2015-10-27 23:06:21]

Description was changed from

==========
Several Site Details / Memory metrics fixes

 - Fix possible O(n^4) worst case behavior (processes x widgets x
   frames x siteinstances) for stats gathering.

 - Show subframe-only processes in about:memory, which also causes them to
   be reported in SiteIsolation.CurrentRendererProcessCount.

 - Because SiteIsolation.CurrentRendererProcessCount is fixed, we need
   a new family of metrics, SiteIsolation.IsolateNothing*, which simulates
   the process-per-siteinstance process model. In tests, IsolateNothing
   is expected to have the value that CurrentRenderProcessCount used to.

 - Implement a more correct treatment of site isolation stats, which looks at
   parent/child transitions and handles ProcessPerSite properly. This is
   required for IsolateNothing to work correctly. Since options for
   walking the frame tree outside of content are pretty limited, this uses
   WebContents::ForEachFrame and (!) memoization.

 - Eliminate WebContents::GetSitesInTab; move its about:blank logic into
   the new frametree walking code in SiteDetails, which was the only caller.

BUG=454526
TEST=browser_tests
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Several Site Details / Memory metrics fixes

 - Fix possible O(n^4) worst case behavior (processes x widgets x
   frames x siteinstances) for stats gathering.

 - Show subframe-only processes in about:memory, which also causes them to
   be reported in SiteIsolation.CurrentRendererProcessCount.

 - Because SiteIsolation.CurrentRendererProcessCount is fixed, we need
   a new family of metrics, SiteIsolation.IsolateNothing*, which simulates
   the process-per-siteinstance process model. In tests, IsolateNothing
   is expected to have the value that CurrentRenderProcessCount used to.

 - Implement a more correct treatment of site isolation stats, which looks at
   parent/child transitions and handles ProcessPerSite properly. This is
   required for IsolateNothing to work correctly. Since options for
   walking the frame tree outside of content are pretty limited, this uses
   WebContents::ForEachFrame and (!) memoization.

 - Eliminate WebContents::GetSitesInTab; move its about:blank logic into
   the new frametree walking code in SiteDetails, which was the only caller.

BUG=454526,542909
TEST=browser_tests
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[ncarter (slow), 2015-10-29 19:49:12]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 19:49:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406133002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406133002/80001

[ncarter (slow), 2015-10-29 19:52:59]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 19:53:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406133002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406133002/100001

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

nick@chromium.org changed reviewers:
+ isherman@chromium.org, jam@chromium.org
- nasko@chromium.org

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

Addressed Charlie's comments

+jam for chrome/ OWNERS
+isherman for histograms.xml OWNERS

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/memory_d...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/memory_d...
chrome/browser/memory_details.cc:299: GURL page_url = contents->GetURL();
On 2015/10/23 19:03:40, Charlie Reis wrote:
> 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.)

Done.

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/memory_d...
chrome/browser/memory_details.cc:342: // make the whole process be considered a
diagnostics process.
On 2015/10/23 19:03:39, Charlie Reis wrote:
> Thank you for moving this comment!  It made no sense where it was before.

Done.

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/site_det...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/site_det...
chrome/browser/site_details.cc:59: void
CollectForScenario(std::map<RenderFrameHost*, GURL>* memo,
On 2015/10/23 19:03:40, Charlie Reis wrote:
> nit: memo -> frame_urls?
> 
> We could add a comment above saying something like:
> Update |scenario|'s data for the given frame.  Memoize each frame's computed
URL
> in |frame_urls| so it can be reused when visiting its children.

Done.

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/site_det...
chrome/browser/site_details.cc:78: if (frame->GetParent()) {
On 2015/10/23 19:03:40, Charlie Reis wrote:
> // Treat a subframe as part of its parent site if neither needs isolation.

Done.

https://codereview.chromium.org/1406133002/diff/20001/chrome/browser/site_det...
chrome/browser/site_details.cc:88: // If we don't need a dedicated process, and
aren't living it a process-
On 2015/10/23 19:03:40, Charlie Reis wrote:
> nit: s/it/in/

Done.

[commit-bot: I haz the power, 2015-10-29 20:08:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-29 20:08:39]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Charlie Reis, 2015-10-29 20:13:42]

https://codereview.chromium.org/1406133002/diff/100001/chrome/browser/memory_...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/1406133002/diff/100001/chrome/browser/memory_...
chrome/browser/memory_details.cc:235: // renderer process belonging to this
browser. All these widgets will share
nit: Merge conflict?  Looks like you lost this line:
// a RenderProcessHost.

[jam, 2015-10-29 20:33:50]

On 2015/10/29 19:55:06, ncarter wrote:
> Addressed Charlie's comments
> 
> +jam for chrome/ OWNERS
can you please reroute to another chrome/owner? a bit backed up.

[Ilya Sherman, 2015-10-29 22:13:36]

metrics lgtm

[ncarter (slow), 2015-10-29 22:14:16]

nick@chromium.org changed reviewers:
+ thestig@chromium.org
- jam@chromium.org

[ncarter (slow), 2015-10-29 22:14:17]

-jam, +thestig for /chrome OWNERS

https://codereview.chromium.org/1406133002/diff/100001/chrome/browser/memory_...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/1406133002/diff/100001/chrome/browser/memory_...
chrome/browser/memory_details.cc:235: // renderer process belonging to this
browser. All these widgets will share
On 2015/10/29 20:13:42, Charlie Reis wrote:
> nit: Merge conflict?  Looks like you lost this line:
> // a RenderProcessHost.

Nice catch, thank you. Done.

[ncarter (slow), 2015-10-29 22:14:36]

The CQ bit was checked by nick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 22:16:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406133002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406133002/120001

[commit-bot: I haz the power, 2015-10-29 23:41:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-29 23:41:12]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Lei Zhang, 2015-10-30 07:17:49]

lgtm, just nits:

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
chrome/browser/memory_details.cc:264: for (std::set<std::string>::iterator iter
= extension_ids.begin();
convert to range-based for loop?

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
chrome/browser/memory_details.cc:301: SiteData* site_data =
Make this a reference and call SiteDetails::CollectSiteInfo(contents,
&site_data) on the next line. Slightly more obvious that |site_data| may be
changing.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
chrome/browser/memory_details.cc:312: #if defined(ENABLE_EXTENSIONS)
I'm probably to blame for the #if ugliness in the old code. This is much better.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
chrome/browser/memory_details.cc:342: // The presence of a single WebContents
with a diagnostics page will make
typo continuing onto next line: make\nmake

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details.cc:28: IsolationScenario* scenario,
const IsolationScenario*

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details.cc:59: // Walk the frame tree and update
|scenario|'s data for the given frame. Memoize
typo: Memoize

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details.cc:75: frame->GetSiteInstance()->GetSiteURL() !=
GURL()) {
Can you call GURL::is_empty() instead?

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details.cc:85: if (!should_isolate &&
!ShouldIsolate(context, scenario, parent_site))
Move the |should_isolate| check up 2 lines?

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:66: return static_cast<int>(count);
Just return size_t and do EXPECT_EQ(1U, foo_ptr->CountPageTitles()) ?

[ncarter (slow), 2015-10-30 18:13:24]

All done.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
chrome/browser/memory_details.cc:264: for (std::set<std::string>::iterator iter
= extension_ids.begin();
On 2015/10/30 07:17:48, Lei Zhang wrote:
> convert to range-based for loop?

Done.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
chrome/browser/memory_details.cc:301: SiteData* site_data =
On 2015/10/30 07:17:49, Lei Zhang wrote:
> Make this a reference and call SiteDetails::CollectSiteInfo(contents,
> &site_data) on the next line. Slightly more obvious that |site_data| may be
> changing.

Done.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
chrome/browser/memory_details.cc:312: #if defined(ENABLE_EXTENSIONS)
On 2015/10/30 07:17:49, Lei Zhang wrote:
> I'm probably to blame for the #if ugliness in the old code. This is much
better.

Thanks!

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/memory_...
chrome/browser/memory_details.cc:342: // The presence of a single WebContents
with a diagnostics page will make
On 2015/10/30 07:17:49, Lei Zhang wrote:
> typo continuing onto next line: make\nmake

Done.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details.cc:28: IsolationScenario* scenario,
On 2015/10/30 07:17:49, Lei Zhang wrote:
> const IsolationScenario*

Done.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details.cc:59: // Walk the frame tree and update
|scenario|'s data for the given frame. Memoize
On 2015/10/30 07:17:49, Lei Zhang wrote:
> typo: Memoize

Not a typo. Memoize is what I meant. https://en.wikipedia.org/wiki/Memoization

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details.cc:75: frame->GetSiteInstance()->GetSiteURL() !=
GURL()) {
On 2015/10/30 07:17:49, Lei Zhang wrote:
> Can you call GURL::is_empty() instead?

Done.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details.cc:85: if (!should_isolate &&
!ShouldIsolate(context, scenario, parent_site))
On 2015/10/30 07:17:49, Lei Zhang wrote:
> Move the |should_isolate| check up 2 lines?

Done.

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1406133002/diff/120001/chrome/browser/site_de...
chrome/browser/site_details_browsertest.cc:66: return static_cast<int>(count);
On 2015/10/30 07:17:49, Lei Zhang wrote:
> Just return size_t and do EXPECT_EQ(1U, foo_ptr->CountPageTitles()) ?

Done.

[ncarter (slow), 2015-10-30 18:13:50]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-10-30 18:13:51]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
thestig@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1406133002/#ps140001
(title: "thestig's fixes.")

[commit-bot: I haz the power, 2015-10-30 18:16:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406133002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406133002/140001

[commit-bot: I haz the power, 2015-10-30 19:25:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 19:25:57]

Try jobs failed on following builders:
  linux_site_isolation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[ncarter (slow), 2015-10-30 19:49:21]

Description was changed from

==========
Several Site Details / Memory metrics fixes

 - Fix possible O(n^4) worst case behavior (processes x widgets x
   frames x siteinstances) for stats gathering.

 - Show subframe-only processes in about:memory, which also causes them to
   be reported in SiteIsolation.CurrentRendererProcessCount.

 - Because SiteIsolation.CurrentRendererProcessCount is fixed, we need
   a new family of metrics, SiteIsolation.IsolateNothing*, which simulates
   the process-per-siteinstance process model. In tests, IsolateNothing
   is expected to have the value that CurrentRenderProcessCount used to.

 - Implement a more correct treatment of site isolation stats, which looks at
   parent/child transitions and handles ProcessPerSite properly. This is
   required for IsolateNothing to work correctly. Since options for
   walking the frame tree outside of content are pretty limited, this uses
   WebContents::ForEachFrame and (!) memoization.

 - Eliminate WebContents::GetSitesInTab; move its about:blank logic into
   the new frametree walking code in SiteDetails, which was the only caller.

BUG=454526,542909
TEST=browser_tests
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Several Site Details / Memory metrics fixes

 - Fix possible O(n^4) worst case behavior (processes x widgets x
   frames x siteinstances) for stats gathering.

 - Show subframe-only processes in about:memory, which also causes them to
   be reported in SiteIsolation.CurrentRendererProcessCount.

 - Because SiteIsolation.CurrentRendererProcessCount is fixed, we need
   a new family of metrics, SiteIsolation.IsolateNothing*, which simulates
   the process-per-siteinstance process model. In tests, IsolateNothing
   is expected to have the value that CurrentRenderProcessCount used to.

 - Implement a more correct treatment of site isolation stats, which looks at
   parent/child transitions and handles ProcessPerSite properly. This is
   required for IsolateNothing to work correctly. Since options for
   walking the frame tree outside of content are pretty limited, this uses
   WebContents::ForEachFrame and (!) memoization.

 - Eliminate WebContents::GetSitesInTab; move its about:blank logic into
   the new frametree walking code in SiteDetails, which was the only caller.

BUG=454526,542909
TEST=browser_tests
==========

[ncarter (slow), 2015-10-30 19:49:30]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-10-30 19:49:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406133002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406133002/140001

[commit-bot: I haz the power, 2015-10-30 20:05:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-30 20:06:32]

Patchset 8 (id:??) landed as
https://crrev.com/b2545f729554c7cbc682504d08ae450155953985
Cr-Commit-Position: refs/heads/master@{#357178}