Native memory histograms for the browser.

Native memory histograms for the browser.

If a command line flag --memory-metrics is passed, the browser measures the
memory consumption after processing each task and puts the data to a histogram.

The histograms will be used in the memory_benchmark of Telemetry.


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

[jochen (gone - plz use gerrit), 2013-01-18 14:03:11]

https://codereview.chromium.org/11975048/diff/5001/content/browser/browser_ma...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/11975048/diff/5001/content/browser/browser_ma...
content/browser/browser_main_loop.cc:229: MemoryObserver() {}
should have a virtual destructor

https://codereview.chromium.org/11975048/diff/5001/content/browser/browser_ma...
content/browser/browser_main_loop.cc:234: LOG(WARNING) << "Adding to browserused
" << webkit_glue::MemoryUsageKB();
remove?

https://codereview.chromium.org/11975048/diff/5001/content/browser/histogram_...
File content/browser/histogram_message_filter.cc (right):

https://codereview.chromium.org/11975048/diff/5001/content/browser/histogram_...
content/browser/histogram_message_filter.cc:31: OnGetBrowserHistogram)
you need to make sure that this is on the IO thread. we can't have sync ipcs on
the ui thread

https://codereview.chromium.org/11975048/diff/5001/content/browser/histogram_...
content/browser/histogram_message_filter.cc:52:
base::StatisticsRecorder::FindHistogram(name);
maybe even add a whitelist?

https://codereview.chromium.org/11975048/diff/5001/content/browser/histogram_...
content/browser/histogram_message_filter.cc:60: }
// namespace content

https://codereview.chromium.org/11975048/diff/5001/content/common/child_proce...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/11975048/diff/5001/content/common/child_proce...
content/common/child_process_messages.h:136:
IPC_SYNC_MESSAGE_CONTROL1_1(ChildProcessHostMsg_GetBrowserHistogram,
move closer to the other histograms IPCs

[marja, 2013-01-18 15:00:08]

Thanks for comments!

https://codereview.chromium.org/11975048/diff/5001/content/browser/browser_ma...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/11975048/diff/5001/content/browser/browser_ma...
content/browser/browser_main_loop.cc:229: MemoryObserver() {}
On 2013/01/18 14:03:11, jochen wrote:
> should have a virtual destructor

Done.

https://codereview.chromium.org/11975048/diff/5001/content/browser/browser_ma...
content/browser/browser_main_loop.cc:234: LOG(WARNING) << "Adding to browserused
" << webkit_glue::MemoryUsageKB();
On 2013/01/18 14:03:11, jochen wrote:
> remove?

Oops.

https://codereview.chromium.org/11975048/diff/5001/content/browser/histogram_...
File content/browser/histogram_message_filter.cc (right):

https://codereview.chromium.org/11975048/diff/5001/content/browser/histogram_...
content/browser/histogram_message_filter.cc:31: OnGetBrowserHistogram)
On 2013/01/18 14:03:11, jochen wrote:
> you need to make sure that this is on the IO thread. we can't have sync ipcs
on
> the ui thread

Added a dcheck in the handler func; it's run in the IO thread.

https://codereview.chromium.org/11975048/diff/5001/content/browser/histogram_...
content/browser/histogram_message_filter.cc:52:
base::StatisticsRecorder::FindHistogram(name);
On 2013/01/18 14:03:11, jochen wrote:
> maybe even add a whitelist?

Done.

https://codereview.chromium.org/11975048/diff/5001/content/browser/histogram_...
content/browser/histogram_message_filter.cc:60: }
On 2013/01/18 14:03:11, jochen wrote:
> // namespace content

Done.

https://codereview.chromium.org/11975048/diff/5001/content/common/child_proce...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/11975048/diff/5001/content/common/child_proce...
content/common/child_process_messages.h:136:
IPC_SYNC_MESSAGE_CONTROL1_1(ChildProcessHostMsg_GetBrowserHistogram,
On 2013/01/18 14:03:11, jochen wrote:
> move closer to the other histograms IPCs

Done.

[jochen (gone - plz use gerrit), 2013-01-18 15:23:25]

sg so far

Please add a BUG :)

[marja, 2013-01-18 15:28:58]

OWNERS {tsepez, sky, jamesr}: Could you review this CL?

tsepez: content/common/child_process_messages.h & related code
sky: content/browser/*
jamesr: content/renderer/*

[marja, 2013-01-18 15:33:05]

Oops, optimizing OWNERS; -sky -jamesr, +creis

OWNERS {tsepez, creis}: Could you review this CL?

tsepez: content/common/child_process_messages.h & related code
creis: everything

[Tom Sepez, 2013-01-18 18:08:01]

Message itself is fine.  Also this is behind a command line switch, will it stay
that way forever?
LGTM for now, but do contact security if this moves out from behind the switch. 
As you add histograms, you run the risk of leaking information about the browser
to the renderer which it has no right to know.

[marja, 2013-01-21 09:28:52]

Thanks for review,

Yes, this is intended to stay behind a command line switch. Measuring the memory
after processing each task is a performance hit, and we are not going to do that
except when running a memory test.

On 2013/01/18 18:08:01, Tom Sepez wrote:
> Message itself is fine.  Also this is behind a command line switch, will it
stay
> that way forever?
> LGTM for now, but do contact security if this moves out from behind the
switch. 
> As you add histograms, you run the risk of leaking information about the
browser
> to the renderer which it has no right to know.

[Charlie Reis, 2013-01-22 23:31:30]

LGTM with nit.  Sorry for the delay!

https://codereview.chromium.org/11975048/diff/4007/content/common/child_proce...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/11975048/diff/4007/content/common/child_proce...
content/common/child_process_messages.h:111: // Request a histogram from the
browser.
nit: Please mention that this is only available behind the memory metrics flag
here (since I had the same concern as Tom about leaking data if that ever
changed).

[marja, 2013-01-23 08:28:44]

Thanks for review!

https://codereview.chromium.org/11975048/diff/4007/content/common/child_proce...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/11975048/diff/4007/content/common/child_proce...
content/common/child_process_messages.h:111: // Request a histogram from the
browser.
On 2013/01/22 23:31:31, creis wrote:
> nit: Please mention that this is only available behind the memory metrics flag
> here (since I had the same concern as Tom about leaking data if that ever
> changed).

Done.

[nduca, 2013-01-23 08:31:33]

I dont quite get the idea of using histograms to pull this data. What about
inserting the memory data into trace events then pulling it from there?

[nduca, 2013-01-23 08:36:33]

+caseq, who might find memory data being inserted into trace_event useful for
devtools

[nduca, 2013-01-31 04:20:11]

telemetry change lgtm

https://codereview.chromium.org/11975048/diff/18001/content/common/child_proc...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/11975048/diff/18001/content/common/child_proc...
content/common/child_process_messages.h:114:
IPC_SYNC_MESSAGE_CONTROL1_1(ChildProcessHostMsg_GetBrowserHistogram,
is there any security risk here with a compromised render process? Effectivley,
all local histograms are exposed via this ipc.

[marja, 2013-01-31 08:19:36]

Thanks for review!

https://codereview.chromium.org/11975048/diff/18001/content/common/child_proc...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/11975048/diff/18001/content/common/child_proc...
content/common/child_process_messages.h:114:
IPC_SYNC_MESSAGE_CONTROL1_1(ChildProcessHostMsg_GetBrowserHistogram,
On 2013/01/31 04:20:11, nduca wrote:
> is there any security risk here with a compromised render process?
Effectivley,
> all local histograms are exposed via this ipc.

The browser only replies with data if it was passed a specific command line
flag, and if the histogram is one of the whitelisted histograms.

[commit-bot: I haz the power, 2013-01-31 08:36:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11975048/22002

[commit-bot: I haz the power, 2013-01-31 08:59:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-31 16:00:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11975048/23002

[commit-bot: I haz the power, 2013-01-31 18:13:56]

Change committed as 179909