Create and pass shared-histogram-allocator from browser to renderer.

Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019
Committed: https://crrev.com/df016a7ce60decb9174ab2201bf4864fc7cefa82
Cr-Commit-Position: refs/heads/master@{#385873}

Committed: https://crrev.com/5370ba54d267e69cc055c75738306644a93454cf
Cr-Commit-Position: refs/heads/master@{#386055}

[bcwhite, 2016-02-23 02:33:00]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org, grt@chromium.org

[bcwhite, 2016-02-23 02:33:00]

This is working on a local build and I can see through the browser histograms
that subprocesses are being attached and their internal histograms being read.

A renderer process that hasn't really done very much has around 100 histograms
to report.

No great rush on detailed review for this as I don't want to submit it until
after the Beta branch but feedback on the design would be appreciated.

[bcwhite, 2016-02-27 18:31:02]

Patchset #3 (id:40001) has been deleted

[bcwhite, 2016-02-27 19:09:54]

Patchset #3 (id:60001) has been deleted

[bcwhite, 2016-02-27 19:18:27]

Patchset #3 (id:80001) has been deleted

[bcwhite, 2016-02-28 02:16:23]

Patchset #4 (id:120001) has been deleted

[bcwhite, 2016-03-23 15:50:26]

Greg and I discussed changing this to pass the handle during process creation
instead of in a message shortly after.

I'd like to delay that to a later CL.  It's unclear how to accomplish this in a
fully platform-independent manner.

Also, the existing mechanism is likely going to have to remain as it's the
fallback should persistent allocation fail for any reason.

[Alexei Svitkine (slow), 2016-03-24 17:29:53]

Some initial comments.

https://codereview.chromium.org/1671933002/diff/140001/base/metrics/histogram...
File base/metrics/histogram_persistence.h (right):

https://codereview.chromium.org/1671933002/diff/140001/base/metrics/histogram...
base/metrics/histogram_persistence.h:49: // on the other side.
Please document the params better. What's id? What's the difference between the
two functions? I suggest having separate comments for them.

https://codereview.chromium.org/1671933002/diff/140001/base/metrics/histogram...
base/metrics/histogram_persistence.h:56:
CreateSharedPersistentHistogramMemoryAllocator(
Overloads are generally discouraged. Please use different names for the two
functions.

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:17: namespace metrics {
The metrics namespace is for code in components/metrics. Since that's not the
case here, please remove it.

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:21: registrar_.Add(this,
content::NOTIFICATION_RENDERER_PROCESS_CREATED,
I think notifications are deprecated. Is there a way to listen for this without
using a notification?

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.h (right):

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.h:58: //
content::NotificationObserver overrides:
Nit: Remove " overrides" here and on line 63.

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.h:84: base::ThreadChecker
thread_checker_;
Nit: Make this the first param.

https://codereview.chromium.org/1671933002/diff/140001/content/child/child_hi...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/1671933002/diff/140001/content/child/child_hi...
content/child/child_histogram_message_filter.cc:56:
base::SetPersistentHistogramMemoryAllocator(allocator.release());
What happens if there's already an allocator set? Will it get lost? If so, adds
some checks against this since it seems we don't want that to happen.

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
File content/public/test/mock_render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
content/public/test/mock_render_process_host.h:97: override;
I think it's pretty strange for the function to be called Get*() but for it to
transfer ownership. Please use a different name that makes it clear that
ownership is being transferred and document the use case in the comment for this
function in the interface.

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
content/public/test/mock_render_process_host.h:146: void SetMetricsAllocator(
What calls this?

[bcwhite, 2016-03-25 15:33:13]

Patchset #5 (id:160001) has been deleted

[bcwhite, 2016-03-29 01:30:17]

Patchset #5 (id:180001) has been deleted

[bcwhite, 2016-03-29 11:53:27]

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:17: namespace metrics {
On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> The metrics namespace is for code in components/metrics. Since that's not the
> case here, please remove it.

Done.

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:21: registrar_.Add(this,
content::NOTIFICATION_RENDERER_PROCESS_CREATED,
On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> I think notifications are deprecated. Is there a way to listen for this
without
> using a notification?

Some notifications are deprecated but I couldn't find anything around the
definition of NOTIFICATION_RENDERER_PROCESS_CREATED indicating that one has
been.

This was the only way I could find.  I originally tried some other way (can't
remember the details) but it wouldn't work across layer boundaries.

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.h (right):

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.h:58: //
content::NotificationObserver overrides:
On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> Nit: Remove " overrides" here and on line 63.

Done.

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.h:84: base::ThreadChecker
thread_checker_;
On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> Nit: Make this the first param.

First param?  To what?  Or do you mean the first member variable?  That doesn't
seem to be the convention elsewhere.

https://codereview.chromium.org/1671933002/diff/140001/content/child/child_hi...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/1671933002/diff/140001/content/child/child_hi...
content/child/child_histogram_message_filter.cc:56:
base::SetPersistentHistogramMemoryAllocator(allocator.release());
On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> What happens if there's already an allocator set? Will it get lost? If so,
adds
> some checks against this since it seems we don't want that to happen.

It dies with a CHECK().

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
File content/public/test/mock_render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
content/public/test/mock_render_process_host.h:97: override;
On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> I think it's pretty strange for the function to be called Get*() but for it to
> transfer ownership. Please use a different name that makes it clear that
> ownership is being transferred and document the use case in the comment for
this
> function in the interface.

Done.

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
content/public/test/mock_render_process_host.h:146: void SetMetricsAllocator(
On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> What calls this?

Nothing.  But since it's a mock and allows extraction of the allocator, it
seemed a good idea to add a method to inject it.

[Alexei Svitkine (slow), 2016-03-29 17:51:25]

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.h (right):

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.h:84: base::ThreadChecker
thread_checker_;
On 2016/03/29 11:53:27, bcwhite wrote:
> On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> > Nit: Make this the first param.
> 
> First param?  To what?  Or do you mean the first member variable?  That
doesn't
> seem to be the convention elsewhere.

Sorry, I did mean first member.

https://codereview.chromium.org/1671933002/diff/140001/content/child/child_hi...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/1671933002/diff/140001/content/child/child_hi...
content/child/child_histogram_message_filter.cc:56:
base::SetPersistentHistogramMemoryAllocator(allocator.release());
On 2016/03/29 11:53:27, bcwhite wrote:
> On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> > What happens if there's already an allocator set? Will it get lost? If so,
> adds
> > some checks against this since it seems we don't want that to happen.
> 
> It dies with a CHECK().

That's fair. Maybe add a comment mentioning that the expectation is that this
message is only received once and that it will trigger a CHECK if it gets called
multiple times.

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
File content/public/test/mock_render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
content/public/test/mock_render_process_host.h:146: void SetMetricsAllocator(
On 2016/03/29 11:53:27, bcwhite wrote:
> On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> > What calls this?
> 
> Nothing.  But since it's a mock and allows extraction of the allocator, it
> seemed a good idea to add a method to inject it.

I don't think we should add it if it's not needed. If it becomes needed, it can
be added by the CL that needs it. Also, you can remove the member variable as
well and make the getter return null.

https://codereview.chromium.org/1671933002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:264: if (!memory->Map(size))
Nit: Combine with the above if and then remove {}'s

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:456:
&browser_sync::ChromeSyncClient::GetDeviceInfoTrackers))));
In the future, please upload separate patchsets for rebases & addressing
comments. Otherwise, when I look at diffs between patchsets, I will see
unrelated changes as a result of the rebase. Thanks!

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:111:
host->ExtractMetricsAllocator();
Please add a comment discussing the lifetime of this.

Specifically, what happens when the renderer goes away? Presumably, we still
want to record its metrics for the last time and then throw away the snapshot
and unmap the memory?

Does that happen somewhere?

Also, what is the lifetime of the shared memory? If UMA is disabled, does it get
unmapped at some point - i.e. by some dtor of some scoped ptr - or does it need
to be done explicitly somewhere? Can you expand the CL description to mention
these details?

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:122:
DCHECK(thread_checker_.CalledOnValidThread());
Nit: Add an empty line after the DCHECKs at the start of the functions. Here and
in the other ones.

https://codereview.chromium.org/1671933002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2602: // Create a
persistent memory segment for renderer histograms only if
Please make a separate function for this block to encapsulate the logic.

https://codereview.chromium.org/1671933002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1671933002/diff/200001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:510:
scoped_ptr<base::SharedPersistentMemoryAllocator> metrics_allocator_;
Needs a comment.

https://codereview.chromium.org/1671933002/diff/200001/content/child/child_hi...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/content/child/child_hi...
content/child/child_histogram_message_filter.cc:49: const
base::SharedMemoryHandle& mem_handle,
Nit: memory_handle and memory_size

https://codereview.chromium.org/1671933002/diff/200001/content/child/child_hi...
File content/child/child_histogram_message_filter.h (right):

https://codereview.chromium.org/1671933002/diff/200001/content/child/child_hi...
content/child/child_histogram_message_filter.h:40: virtual void
OnSetHistogramMemory(const base::SharedMemoryHandle& mem_handle,
Does this need to be virtual? If it doesn't probably the one below doesn't
either.

https://codereview.chromium.org/1671933002/diff/200001/content/common/child_p...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/1671933002/diff/200001/content/common/child_p...
content/common/child_process_messages.h:107: // processed.
Please add a note about the histograms that are created before this message is
received - i.e. they will still be retrieved via GetChildHistogramData.

Maybe we should change the name of the other function to something like
GetChildNonPersistentHistogramData()?

https://codereview.chromium.org/1671933002/diff/200001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/200001/content/public/browser...
content/public/browser/render_process_host.h:263: // Ownership is passed to th
ecaller. To support sharing of histogram data
Nit: "th ecaller"

[bcwhite, 2016-03-30 21:25:24]

Description was changed from

==========
Create and pass shared-histogram-allocator from browser to renderer.

BUG=546019
==========

to

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019
==========

[bcwhite, 2016-03-30 21:25:56]

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.h (right):

https://codereview.chromium.org/1671933002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.h:84: base::ThreadChecker
thread_checker_;
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> On 2016/03/29 11:53:27, bcwhite wrote:
> > On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> > > Nit: Make this the first param.
> > 
> > First param?  To what?  Or do you mean the first member variable?  That
> doesn't
> > seem to be the convention elsewhere.
> 
> Sorry, I did mean first member.

Done.  Though like I said, there doesn't seem to be any regular practice of
doing that.

https://codereview.chromium.org/1671933002/diff/140001/content/child/child_hi...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/1671933002/diff/140001/content/child/child_hi...
content/child/child_histogram_message_filter.cc:56:
base::SetPersistentHistogramMemoryAllocator(allocator.release());
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> On 2016/03/29 11:53:27, bcwhite wrote:
> > On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> > > What happens if there's already an allocator set? Will it get lost? If so,
> > adds
> > > some checks against this since it seems we don't want that to happen.
> > 
> > It dies with a CHECK().
> 
> That's fair. Maybe add a comment mentioning that the expectation is that this
> message is only received once and that it will trigger a CHECK if it gets
called
> multiple times.

Done.

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
File content/public/test/mock_render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/140001/content/public/test/mo...
content/public/test/mock_render_process_host.h:146: void SetMetricsAllocator(
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> On 2016/03/29 11:53:27, bcwhite wrote:
> > On 2016/03/24 17:29:53, Alexei Svitkine (OOO 25-27th) wrote:
> > > What calls this?
> > 
> > Nothing.  But since it's a mock and allows extraction of the allocator, it
> > seemed a good idea to add a method to inject it.
> 
> I don't think we should add it if it's not needed. If it becomes needed, it
can
> be added by the CL that needs it. Also, you can remove the member variable as
> well and make the getter return null.

Done.

https://codereview.chromium.org/1671933002/diff/200001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:264: if (!memory->Map(size))
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> Nit: Combine with the above if and then remove {}'s

Done.

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:456:
&browser_sync::ChromeSyncClient::GetDeviceInfoTrackers))));
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> In the future, please upload separate patchsets for rebases & addressing
> comments. Otherwise, when I look at diffs between patchsets, I will see
> unrelated changes as a result of the rebase. Thanks!

Acknowledged.

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:111:
host->ExtractMetricsAllocator();
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> Please add a comment discussing the lifetime of this.
> 
> Specifically, what happens when the renderer goes away? Presumably, we still
> want to record its metrics for the last time and then throw away the snapshot
> and unmap the memory?
> 
> Does that happen somewhere?
> 
> Also, what is the lifetime of the shared memory? If UMA is disabled, does it
get
> unmapped at some point - i.e. by some dtor of some scoped ptr - or does it
need
> to be done explicitly somewhere? Can you expand the CL description to mention
> these details?

Done.  Good point about the UMA not being enabled, though.  Easy fix: just don't
register it and let it be deleted by the scoped_ptr going out-of-scope.

How can I detect, here, if UMA is enabled?

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:122:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> Nit: Add an empty line after the DCHECKs at the start of the functions. Here
and
> in the other ones.

Done.

https://codereview.chromium.org/1671933002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2602: // Create a
persistent memory segment for renderer histograms only if
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> Please make a separate function for this block to encapsulate the logic.

Done.

https://codereview.chromium.org/1671933002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1671933002/diff/200001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:510:
scoped_ptr<base::SharedPersistentMemoryAllocator> metrics_allocator_;
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> Needs a comment.

Done.

https://codereview.chromium.org/1671933002/diff/200001/content/child/child_hi...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/content/child/child_hi...
content/child/child_histogram_message_filter.cc:49: const
base::SharedMemoryHandle& mem_handle,
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> Nit: memory_handle and memory_size

Done.

https://codereview.chromium.org/1671933002/diff/200001/content/child/child_hi...
File content/child/child_histogram_message_filter.h (right):

https://codereview.chromium.org/1671933002/diff/200001/content/child/child_hi...
content/child/child_histogram_message_filter.h:40: virtual void
OnSetHistogramMemory(const base::SharedMemoryHandle& mem_handle,
On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> Does this need to be virtual? If it doesn't probably the one below doesn't
> either.

Done.  It looks like it was done that way to match the other OnFoo handlers that
are inherited virtual from IPC::MessageFilter.

https://codereview.chromium.org/1671933002/diff/200001/content/common/child_p...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/1671933002/diff/200001/content/common/child_p...
content/common/child_process_messages.h:107: // processed.
On 2016/03/29 17:51:25, Alexei Svitkine wrote:
> Please add a note about the histograms that are created before this message is
> received - i.e. they will still be retrieved via GetChildHistogramData.

Done.

> 
> Maybe we should change the name of the other function to something like
> GetChildNonPersistentHistogramData()?

Done.

https://codereview.chromium.org/1671933002/diff/200001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/200001/content/public/browser...
content/public/browser/render_process_host.h:263: // Ownership is passed to th
ecaller. To support sharing of histogram data
On 2016/03/29 17:51:25, Alexei Svitkine wrote:
> Nit: "th ecaller"

Done.

[Alexei Svitkine (slow), 2016-03-31 15:22:04]

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:111:
host->ExtractMetricsAllocator();
On 2016/03/30 21:25:55, bcwhite wrote:
> On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> > Please add a comment discussing the lifetime of this.
> > 
> > Specifically, what happens when the renderer goes away? Presumably, we still
> > want to record its metrics for the last time and then throw away the
snapshot
> > and unmap the memory?
> > 
> > Does that happen somewhere?
> > 
> > Also, what is the lifetime of the shared memory? If UMA is disabled, does it
> get
> > unmapped at some point - i.e. by some dtor of some scoped ptr - or does it
> need
> > to be done explicitly somewhere? Can you expand the CL description to
mention
> > these details?
> 
> Done.  Good point about the UMA not being enabled, though.  Easy fix: just
don't
> register it and let it be deleted by the scoped_ptr going out-of-scope.
> 
> How can I detect, here, if UMA is enabled?

You can implement the OnRecordingEnabled() and OnRecordingDisabled() functions
and keep track of the state.

https://codereview.chromium.org/1671933002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1671933002/diff/220001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2411: if
(metrics_allocator_) {
This if shouldn't be necessary, since a new call above should never return null.

[bcwhite, 2016-03-31 18:25:47]

Patchset #7 (id:240001) has been deleted

[bcwhite, 2016-03-31 18:27:49]

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:111:
host->ExtractMetricsAllocator();
On 2016/03/31 15:22:04, Alexei Svitkine wrote:
> On 2016/03/30 21:25:55, bcwhite wrote:
> > On 2016/03/29 17:51:24, Alexei Svitkine wrote:
> > > Please add a comment discussing the lifetime of this.
> > > 
> > > Specifically, what happens when the renderer goes away? Presumably, we
still
> > > want to record its metrics for the last time and then throw away the
> snapshot
> > > and unmap the memory?
> > > 
> > > Does that happen somewhere?
> > > 
> > > Also, what is the lifetime of the shared memory? If UMA is disabled, does
it
> > get
> > > unmapped at some point - i.e. by some dtor of some scoped ptr - or does it
> > need
> > > to be done explicitly somewhere? Can you expand the CL description to
> mention
> > > these details?
> > 
> > Done.  Good point about the UMA not being enabled, though.  Easy fix: just
> don't
> > register it and let it be deleted by the scoped_ptr going out-of-scope.
> > 
> > How can I detect, here, if UMA is enabled?
> 
> You can implement the OnRecordingEnabled() and OnRecordingDisabled() functions
> and keep track of the state.

Done.

https://codereview.chromium.org/1671933002/diff/220001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1671933002/diff/220001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2411: if
(metrics_allocator_) {
On 2016/03/31 15:22:04, Alexei Svitkine wrote:
> This if shouldn't be necessary, since a new call above should never return
null.

Done.

[Alexei Svitkine (slow), 2016-03-31 22:00:58]

Looks great, just some comments about histograms and then I think it's ready to
go. :)

https://codereview.chromium.org/1671933002/diff/260001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/260001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:105:
"UMA.SubprocessMetricsProvider.SubproccessCount",
Add this to histograms.xml please.

https://codereview.chromium.org/1671933002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1671933002/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2409:
"RendererMetrics", /*readonly=*/false));
Do you need to define any histograms using this new name?

https://codereview.chromium.org/1671933002/diff/260001/content/child/child_hi...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/1671933002/diff/260001/content/child/child_hi...
content/child/child_histogram_message_filter.cc:58: if (global_allocator) {
Nit: No {}'s

[bcwhite, 2016-03-31 23:53:47]

https://codereview.chromium.org/1671933002/diff/260001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/260001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:105:
"UMA.SubprocessMetricsProvider.SubproccessCount",
On 2016/03/31 22:00:57, Alexei Svitkine wrote:
> Add this to histograms.xml please.

Done.

https://codereview.chromium.org/1671933002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1671933002/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2409:
"RendererMetrics", /*readonly=*/false));
On 2016/03/31 22:00:58, Alexei Svitkine wrote:
> Do you need to define any histograms using this new name?

There are two already in the CL:
UMA.PersistentAllocator.RendererMetrics.Allocs
UMA.PersistentAllocator.RendererMetrics.UsedPct

https://codereview.chromium.org/1671933002/diff/260001/content/child/child_hi...
File content/child/child_histogram_message_filter.cc (right):

https://codereview.chromium.org/1671933002/diff/260001/content/child/child_hi...
content/child/child_histogram_message_filter.cc:58: if (global_allocator) {
On 2016/03/31 22:00:58, Alexei Svitkine wrote:
> Nit: No {}'s

Done.

[grt (UTC plus 2), 2016-04-01 14:22:49]

It appears that this requires more work up in src/chrome than the polling
mechanism, which largely lives in src/content. Can more of the impl be pushed
down into content?

I think it is desirable for the UMA shared memory stuff to be a fundamental part
of the subprocess model rather than strapped on at the application layer. How
hard would this be?

https://codereview.chromium.org/1671933002/diff/280001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1671933002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:9: #include
"base/memory/scoped_ptr.h"
remove these two -- already included in .h

https://codereview.chromium.org/1671933002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:12: #include
"base/metrics/histogram_base.h"
remove this -- already included in .h

https://codereview.chromium.org/1671933002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:278: scoped_ptr<SharedMemory>
shm(new SharedMemory(handle, /*readonly=*/false));
iiuc, this effectively takes ownership of |handle| since
SharedMemory::SharedMemory copies the handle value and
SharedMemory::~SharedMemory will eventually close it. in this case, i think it's
deceptive to take |handle| as a constref since the caller can no longer safely
use the thing they pass in. please do something so that it's impossible to
mis-use.

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:151:
DeregisterSubprocessAllocator(host->GetID());
since RegisterSubprocessAllocator happens once the subprocess has spun up (in
RenderProcessReady), the symmetrical place to Deregister seems to be in
RenderProcessExited rather than RenderProcessHostDestroyed. in particular, the
doc comment on RenderProcessExited leads me to believe that another sub-process
could be spun up for the same RPH, in which case you'd currently handle more
than one "Ready" call where you register multiple times with the same ID.

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.h (right):

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.h:29: public
content::RenderProcessHostObserver {
rather than being a generic subprocess metrics provider, this is currently a
renderer subprocess metrics provider. is it possible to make it operate down at
the level of content::ChildProcessHost (the basis for renderers and other
subprocs)? if not, please rename it so that the class name makes it clear that
it only applies to renderers.

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider_unittest.cc:19: class
HistogramFlattenerDeltaRecorder : public base::HistogramFlattener {
nit: put this helper into the unnamed namespace

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider_unittest.cc:56: void SetUp()
override {
nit: prefer the ctor/dtor pair over SetUp/TearDown when possible
(https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...).

https://codereview.chromium.org/1671933002/diff/280001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1671933002/diff/280001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:158:
scoped_ptr<base::SharedPersistentMemoryAllocator> ExtractMetricsAllocator()
nit: TakeMetricsAllocator() for consistency with other functions that pass
ownership of a thing to the caller.

https://codereview.chromium.org/1671933002/diff/280001/content/common/child_p...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/1671933002/diff/280001/content/common/child_p...
content/common/child_process_messages.h:106: // (GetChildHistogramData) will
return any histograms created before this
GetChildNonPersistentHistogramData

https://codereview.chromium.org/1671933002/diff/280001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/280001/content/public/browser...
content/public/browser/render_process_host.h:268: ExtractMetricsAllocator() = 0;
Take

[bcwhite, 2016-04-01 16:29:10]

Patchset #9 (id:300001) has been deleted

[bcwhite, 2016-04-01 16:50:26]

As per face-to-face discussion with Greg, efforts to move this up to the content
layer and interface directly with the browser_child_process_host will be done in
a future CL so as to not hold-up this CL (which others are waiting for).

https://codereview.chromium.org/1671933002/diff/280001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1671933002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:9: #include
"base/memory/scoped_ptr.h"
On 2016/04/01 14:22:48, grt (very slow) wrote:
> remove these two -- already included in .h

Done.

https://codereview.chromium.org/1671933002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:12: #include
"base/metrics/histogram_base.h"
On 2016/04/01 14:22:48, grt (very slow) wrote:
> remove this -- already included in .h

Done.

https://codereview.chromium.org/1671933002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:278: scoped_ptr<SharedMemory>
shm(new SharedMemory(handle, /*readonly=*/false));
On 2016/04/01 14:22:48, grt (very slow) wrote:
> iiuc, this effectively takes ownership of |handle| since
> SharedMemory::SharedMemory copies the handle value and
> SharedMemory::~SharedMemory will eventually close it. in this case, i think
it's
> deceptive to take |handle| as a constref since the caller can no longer safely
> use the thing they pass in. please do something so that it's impossible to
> mis-use.

The passing convention matches that of the SharedMemory object to which it is
given.  I could switch it to a std::move(scoped_ptr) pattern here but it seems a
bit excessive, complicate the creation by creating a duplicate of something
passed by value, and will be at odds with the conventions of the underlying
class.

I'll add a comment but don't see any simple way to prevent mis-use in code.

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:151:
DeregisterSubprocessAllocator(host->GetID());
On 2016/04/01 14:22:48, grt (very slow) wrote:
> since RegisterSubprocessAllocator happens once the subprocess has spun up (in
> RenderProcessReady), the symmetrical place to Deregister seems to be in
> RenderProcessExited rather than RenderProcessHostDestroyed. in particular, the
> doc comment on RenderProcessExited leads me to believe that another
sub-process
> could be spun up for the same RPH, in which case you'd currently handle more
> than one "Ready" call where you register multiple times with the same ID.

I had that originally but there are cases where a renderer would terminate but
not perform that callback whereas Destroyed is always called.  Perhaps the best
choice is to respond to both; the code is already tolerant if trying to
deregister something that isn't known.

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.h (right):

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.h:29: public
content::RenderProcessHostObserver {
On 2016/04/01 14:22:48, grt (very slow) wrote:
> rather than being a generic subprocess metrics provider, this is currently a
> renderer subprocess metrics provider. is it possible to make it operate down
at
> the level of content::ChildProcessHost (the basis for renderers and other
> subprocs)? if not, please rename it so that the class name makes it clear that
> it only applies to renderers.

It doesn't apply only to renderers.  It's just that only the code for renderers
has currently been implemented.  The plan is to extend it to other processes in
future CLs.

As discussed in person, I'll investigate in a later CL at shifting this to a
lower layer and interface at the process level.

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider_unittest.cc:19: class
HistogramFlattenerDeltaRecorder : public base::HistogramFlattener {
On 2016/04/01 14:22:48, grt (very slow) wrote:
> nit: put this helper into the unnamed namespace

Done.

https://codereview.chromium.org/1671933002/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider_unittest.cc:56: void SetUp()
override {
On 2016/04/01 14:22:48, grt (very slow) wrote:
> nit: prefer the ctor/dtor pair over SetUp/TearDown when possible
>
(https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...).

Done.

https://codereview.chromium.org/1671933002/diff/280001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/1671933002/diff/280001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.h:158:
scoped_ptr<base::SharedPersistentMemoryAllocator> ExtractMetricsAllocator()
On 2016/04/01 14:22:49, grt (very slow) wrote:
> nit: TakeMetricsAllocator() for consistency with other functions that pass
> ownership of a thing to the caller.

Done.

https://codereview.chromium.org/1671933002/diff/280001/content/common/child_p...
File content/common/child_process_messages.h (right):

https://codereview.chromium.org/1671933002/diff/280001/content/common/child_p...
content/common/child_process_messages.h:106: // (GetChildHistogramData) will
return any histograms created before this
On 2016/04/01 14:22:49, grt (very slow) wrote:
> GetChildNonPersistentHistogramData

Done.

https://codereview.chromium.org/1671933002/diff/280001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/280001/content/public/browser...
content/public/browser/render_process_host.h:268: ExtractMetricsAllocator() = 0;
On 2016/04/01 14:22:49, grt (very slow) wrote:
> Take

Done.

[Alexei Svitkine (slow), 2016-04-01 16:55:41]

lgtm

[Alexei Svitkine (slow), 2016-04-01 16:56:28]

lgtm

https://codereview.chromium.org/1671933002/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1671933002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:54278: +<histogram
name="UMA.SubprocessMetricsProvider.SubprocessCount" units="count">
Nit: units="subprocesses"

https://codereview.chromium.org/1671933002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:54283: +    during the last reporting
cycle.
Nit: Mention at what point this is logged.

[bcwhite, 2016-04-01 17:37:59]

https://codereview.chromium.org/1671933002/diff/320001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1671933002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:54278: +<histogram
name="UMA.SubprocessMetricsProvider.SubprocessCount" units="count">
On 2016/04/01 16:56:28, Alexei Svitkine wrote:
> Nit: units="subprocesses"

Done.

https://codereview.chromium.org/1671933002/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:54283: +    during the last reporting
cycle.
On 2016/04/01 16:56:28, Alexei Svitkine wrote:
> Nit: Mention at what point this is logged.

Done.

[grt (UTC plus 2), 2016-04-01 20:45:33]

https://codereview.chromium.org/1671933002/diff/320001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:154:
scoped_observer_.Remove(host);
should this removal be here? don't you need to keep observing in case the RPH
spins up a new subprocess?

[bcwhite, 2016-04-01 20:47:29]

bcwhite@chromium.org changed reviewers:
+ jschuh@chromium.org

[bcwhite, 2016-04-01 20:47:30]

jschuh@chromium.org: Please review changes in 

content/common/child_process_messages.h

Justin, this enables the shared memory between the Browser and Renderer as
discussed in this design-doc:
https://drive.google.com/open?id=1YvBXzi745UDjiFVLjP8IWUMFgYL9gDb2J4xuwcv-c7o

All of the hardening to protect against crashes, both in the allocator and in
the histograms code has been done.  Data transfer is one-way
(renderer-to-browser) with no sharing between renderers.

So there should be no way for a compromised renderer to cause a crash in the
browser or gain access to any more information than it already has today.

[jschuh, 2016-04-01 22:09:39]

Sorry, I'm not going to have time to look at this. Maybe wfh@?

[grt (UTC plus 2), 2016-04-04 16:46:44]

https://codereview.chromium.org/1671933002/diff/320001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:154:
scoped_observer_.Remove(host);
On 2016/04/01 20:45:33, grt (very slow) wrote:
> should this removal be here? don't you need to keep observing in case the RPH
> spins up a new subprocess?

ping

[bcwhite, 2016-04-04 19:32:27]

https://codereview.chromium.org/1671933002/diff/320001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider.cc (right):

https://codereview.chromium.org/1671933002/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider.cc:154:
scoped_observer_.Remove(host);
On 2016/04/04 16:46:44, grt (very slow) wrote:
> On 2016/04/01 20:45:33, grt (very slow) wrote:
> > should this removal be here? don't you need to keep observing in case the
RPH
> > spins up a new subprocess?
> 
> ping

Not forgotten.  Just working on other CLs.

[bcwhite, 2016-04-04 19:36:07]

bcwhite@chromium.org changed reviewers:
+ wfh@chromium.org
- jschuh@chromium.org

[bcwhite, 2016-04-06 19:40:29]

Patchset #12 (id:380001) has been deleted

[bcwhite, 2016-04-06 19:52:26]

Patchset #12 (id:400001) has been deleted

[bcwhite, 2016-04-06 19:52:44]

Patchset #12 (id:420001) has been deleted

[bcwhite, 2016-04-06 20:30:52]

Patchset #12 (id:440001) has been deleted

[Will Harris, 2016-04-06 23:38:00]

I am reviewing now... you might want to change all your scoped_ptr since the
scoped_ptr -> std::unique_ptr changes landed (I see you rebased them already in
ps12)

[bcwhite, 2016-04-06 23:51:45]

On 2016/04/06 23:38:00, Will Harris wrote:
> I am reviewing now... you might want to change all your scoped_ptr since the
> scoped_ptr -> std::unique_ptr changes landed (I see you rebased them already
in
> ps12)

Will do.

[Will Harris, 2016-04-07 00:29:41]

Few questions:

Can you briefly explain when RecordHistogramSnapshots is called from the
snapshot manager?

I don't quite understand the movement of the allocators from the active to the
allocators_to_release_ list, to finally being freed. When exactly does this
happen?

I'm concerned about untrusted data from renderers being parsed in the browser
process, this parsing seems to take place in histogram_snapshot_manager.cc? What
testing or assurance have you done to determine that the security guarantees you
mention in the document (e.g. failing safely on corrupted data) are being upheld
here?

Do you have a fuzzer for the input to the HistogramSnapshotManager?

https://codereview.chromium.org/1671933002/diff/460001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1671933002/diff/460001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider_unittest.cc:60: //
RecordHistogramSnapshots needs to becalled beause it uses a histogram
nit: space

https://codereview.chromium.org/1671933002/diff/460001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1671933002/diff/460001/components/metrics/met...
components/metrics/metrics_service.cc:138: #include
"base/metrics/persistent_memory_allocator.h"
is this needed?

https://codereview.chromium.org/1671933002/diff/460001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1671933002/diff/460001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2384: std::move(shm),
child_process_launcher_->GetProcess().Pid(),
use RenderProcessHostImpl::GetHandle here

also, do you plan to handle run_renderer_in_process() being true, if so you
might not have a child_process_launcher_

https://codereview.chromium.org/1671933002/diff/460001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/460001/content/public/browser...
content/public/browser/render_process_host.h:264: TakeMetricsAllocator() = 0;
cl format?

[bcwhite, 2016-04-07 01:15:55]

> Can you briefly explain when RecordHistogramSnapshots
> is called from the snapshot manager?

It's called from MetricsService as part of the periodic collection of histograms
for upload.

> I don't quite understand the movement of the
> allocators from the active to the
> allocators_to_release_ list, to finally being freed.
> When exactly does this happen?

It's moved when a renderer terminates.  The memory segment has to be kept alive
until the next reporting cycle is done so that metrics don't get lost.  Once a
reporting cycle is complete and any final deltas collected, the shared memory
allocator can be released.


> I'm concerned about untrusted data from renderers
> being parsed in the browser process, this parsing
> seems to take place in histogram_snapshot_manager.cc?
> What testing or assurance have you done to determine
> that the security guarantees you mention in the
> document (e.g. failing safely on corrupted data) are
> being upheld here?

The Allocator itself has a "fuzzer" test to validate it's resilience against
corruption in the memory.

Loading histograms validates every parameter as being acceptable before actually
creating the histogram from the local heap (but pointing to the samples held in
shared memory).  Once it does, it no longer matters what a compromised renderer
may do because the only cross-talk is reading and writing samples which, though
they could be garbage, aren't interpreted in any way that could cause a crash.


> Do you have a fuzzer for the input to the
> HistogramSnapshotManager?

No but it operates on objects created safely inside the local browser process. 
They read the sample data from the shared memory for calculating a delta but
though those values could be garbage, they don't affect operation and so can't
crash.

The susceptibility to bad data is unchanged.  Even today, Renderers send their
deltas via IPC without regard to correctness.

I'll look into writing a fuzzer to additional validation of the parsing of
histogram data from a shared segment but I'd prefer to do that as a separate CL
so as to not delay this CL.  This feature won't be going all the way to stable
in M50 anyway.

https://codereview.chromium.org/1671933002/diff/460001/chrome/browser/metrics...
File chrome/browser/metrics/subprocess_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1671933002/diff/460001/chrome/browser/metrics...
chrome/browser/metrics/subprocess_metrics_provider_unittest.cc:60: //
RecordHistogramSnapshots needs to becalled beause it uses a histogram
On 2016/04/07 00:29:41, Will Harris wrote:
> nit: space

Done.

https://codereview.chromium.org/1671933002/diff/460001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1671933002/diff/460001/components/metrics/met...
components/metrics/metrics_service.cc:138: #include
"base/metrics/persistent_memory_allocator.h"
On 2016/04/07 00:29:41, Will Harris wrote:
> is this needed?

Done.

https://codereview.chromium.org/1671933002/diff/460001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1671933002/diff/460001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:2384: std::move(shm),
child_process_launcher_->GetProcess().Pid(),
On 2016/04/07 00:29:41, Will Harris wrote:
> use RenderProcessHostImpl::GetHandle here

It looks like GetId() is a better choice; I just need some sort of identifying
number.

> also, do you plan to handle run_renderer_in_process() being true, if
> so you might not have a child_process_launcher_

Check added.

https://codereview.chromium.org/1671933002/diff/460001/content/public/browser...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/1671933002/diff/460001/content/public/browser...
content/public/browser/render_process_host.h:264: TakeMetricsAllocator() = 0;
On 2016/04/07 00:29:41, Will Harris wrote:
> cl format?

That's the way it likes it.  I originally had extra indent on the second and it
took them out.

[Will Harris, 2016-04-07 19:53:55]

security lgtm.

[bcwhite, 2016-04-07 20:01:47]

bcwhite@chromium.org changed reviewers:
+ avi@chromium.org

[bcwhite, 2016-04-07 20:01:48]

avi@chromium.org: Please review changes in 

content/

[Avi (use Gerrit), 2016-04-07 20:14:19]

lgtm

[bcwhite, 2016-04-07 20:15:23]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-04-07 20:15:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1671933002/#ps500001
(title: "addressed review comments by Will")

[commit-bot: I haz the power, 2016-04-07 20:16:19]

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

[commit-bot: I haz the power, 2016-04-07 21:30:14]

Description was changed from

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019
==========

to

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019
==========

[commit-bot: I haz the power, 2016-04-07 21:30:16]

Committed patchset #14 (id:500001)

[commit-bot: I haz the power, 2016-04-07 21:32:39]

Description was changed from

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019
==========

to

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019

Committed: https://crrev.com/df016a7ce60decb9174ab2201bf4864fc7cefa82
Cr-Commit-Position: refs/heads/master@{#385873}
==========

[commit-bot: I haz the power, 2016-04-07 21:32:41]

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

[bcwhite, 2016-04-08 01:43:57]

Looks like Android doesn't like how the memory is duplicated for testing.  Will
revert, change the test, and re-submit.

[bcwhite, 2016-04-08 01:44:24]

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20GN/b...

[bcwhite, 2016-04-08 01:49:01]

Description was changed from

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019

Committed: https://crrev.com/df016a7ce60decb9174ab2201bf4864fc7cefa82
Cr-Commit-Position: refs/heads/master@{#385873}
==========

to

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019

Committed: https://crrev.com/df016a7ce60decb9174ab2201bf4864fc7cefa82
Cr-Commit-Position: refs/heads/master@{#385873}
==========

[bcwhite, 2016-04-08 12:37:11]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-04-08 12:37:11]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
asvitkine@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1671933002/#ps520001
(title: "fixed test to not use SharedMemory which doesn't work on all builds
(and rebased)")

[commit-bot: I haz the power, 2016-04-08 12:37:17]

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

[commit-bot: I haz the power, 2016-04-08 12:44:11]

Description was changed from

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019

Committed: https://crrev.com/df016a7ce60decb9174ab2201bf4864fc7cefa82
Cr-Commit-Position: refs/heads/master@{#385873}
==========

to

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019

Committed: https://crrev.com/df016a7ce60decb9174ab2201bf4864fc7cefa82
Cr-Commit-Position: refs/heads/master@{#385873}
==========

[commit-bot: I haz the power, 2016-04-08 12:44:12]

Committed patchset #15 (id:520001)

[commit-bot: I haz the power, 2016-04-08 12:45:23]

Description was changed from

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019

Committed: https://crrev.com/df016a7ce60decb9174ab2201bf4864fc7cefa82
Cr-Commit-Position: refs/heads/master@{#385873}
==========

to

==========
Create and pass shared-histogram-allocator from browser to renderer.

After the renderer is started, a shared-memory segment
is created and sent to it; all renderer histograms
created after that point will be stored there.

The SubprocessMetricsProvider extracts that shared
segment on the browser side and, when metrics are being
uploaded, iterates through all the histograms stored
there.

When the renderer exits, the SMP will continue to hold
a handle to the memory segment.  Once all the histograms
are uploaded (during the next cycle), it will finally
release the memory back to the operating system.

BUG=546019

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

Committed: https://crrev.com/5370ba54d267e69cc055c75738306644a93454cf
Cr-Commit-Position: refs/heads/master@{#386055}
==========

[commit-bot: I haz the power, 2016-04-08 12:45:24]

Patchset 15 (id:??) landed as
https://crrev.com/5370ba54d267e69cc055c75738306644a93454cf
Cr-Commit-Position: refs/heads/master@{#386055}