Introduce mojo interface for collecting StackSamplingProfiler data

Introduce mojo interface for collecting StackSamplingProfiler data

Adds interfaces, typemaps, and tests for collecting
StackSamplingProfiler result types. The collection interface will be
used to receive stack profiles from non-browser processes.

BUG=517958
Committed: https://crrev.com/551e14b666639639850b2de5588b30b4e40d626b
Cr-Commit-Position: refs/heads/master@{#412963}

[Mike Wittman, 2016-08-17 20:23:37]

Description was changed from

==========
Introduce mojo interface for collecting StackSamplingProfiler data

Adds interfaces, typemaps, and tests for collecting
StackSamplingProfiler result types. The collection interface will be
used to receive stack profiles from non-browser processes.

BUG=517958
==========

to

==========
Introduce mojo interface for collecting StackSamplingProfiler data

Adds interfaces, typemaps, and tests for collecting
StackSamplingProfiler result types. The collection interface will be
used to receive stack profiles from non-browser processes.

BUG=517958
==========

[Mike Wittman, 2016-08-17 20:23:37]

wittman@chromium.org changed reviewers:
+ isherman@chromium.org, sky@chromium.org, tsepez@chromium.org

[Mike Wittman, 2016-08-17 20:25:40]

PTAL

sky: *
isherman: *
tsepez: mojom, struct_traits

[sky, 2016-08-17 21:01:33]

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
File components/metrics/call_stack_profile_collector.mojom (right):

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
components/metrics/call_stack_profile_collector.mojom:7: import
"mojo/common/common_custom_types.mojom";
Generally we put mojoms in a directory named public/interfaces. Search for other
mojoms in components to see what I mean.

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
File components/metrics/call_stack_profile_struct_traits.h (right):

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
components/metrics/call_stack_profile_struct_traits.h:150:
base::Optional<std::vector<base::StackSamplingProfiler::Module>> modules;
How come this is optional? The mojom doesn't have the fields as optional, so I
wouldn't expect an optional here.

[Tom Sepez, 2016-08-17 21:53:08]

lgtm

[Mike Wittman, 2016-08-17 22:38:06]

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

[commit-bot: I haz the power, 2016-08-17 22:38:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike Wittman, 2016-08-17 22:40:15]

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
File components/metrics/call_stack_profile_collector.mojom (right):

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
components/metrics/call_stack_profile_collector.mojom:7: import
"mojo/common/common_custom_types.mojom";
On 2016/08/17 21:01:33, sky wrote:
> Generally we put mojoms in a directory named public/interfaces. Search for
other
> mojoms in components to see what I mean.

Moved the files under public. I don't see a reason to diverge from convention
here.

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
File components/metrics/call_stack_profile_struct_traits.h (right):

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
components/metrics/call_stack_profile_struct_traits.h:150:
base::Optional<std::vector<base::StackSamplingProfiler::Module>> modules;
On 2016/08/17 21:01:33, sky wrote:
> How come this is optional? The mojom doesn't have the fields as optional, so I
> wouldn't expect an optional here.

I'm using Optional to determine if these fields have been created yet in the
modules() and samples() functions.

[commit-bot: I haz the power, 2016-08-17 22:46:11]

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

[commit-bot: I haz the power, 2016-08-17 22:46:12]

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

[Mike Wittman, 2016-08-17 23:20:57]

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
File components/metrics/call_stack_profile_struct_traits.h (right):

https://codereview.chromium.org/2254743003/diff/1/components/metrics/call_sta...
components/metrics/call_stack_profile_struct_traits.h:150:
base::Optional<std::vector<base::StackSamplingProfiler::Module>> modules;
On 2016/08/17 22:40:15, Mike Wittman wrote:
> On 2016/08/17 21:01:33, sky wrote:
> > How come this is optional? The mojom doesn't have the fields as optional, so
I
> > wouldn't expect an optional here.
> 
> I'm using Optional to determine if these fields have been created yet in the
> modules() and samples() functions.

Actually, I don't believe I need Context at all... This was left over from when
I was anticipating using a move interface.

[Mike Wittman, 2016-08-17 23:24:11]

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

[commit-bot: I haz the power, 2016-08-17 23:24:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-08-17 23:41:14]

Nice, LGTM

[commit-bot: I haz the power, 2016-08-18 00:33:38]

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

[commit-bot: I haz the power, 2016-08-18 00:33:39]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ilya Sherman, 2016-08-18 07:59:18]

LGTM % comments.  I'm minimally fluent in Mojo, so apologies if some/all of the
comments below don't make sense...

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
File components/metrics/public/cpp/call_stack_profile_struct_traits.h (right):

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits.h:39: if
(!data.ReadId(&id) || !data.ReadFilename(&filename))
Tom didn't comment on this, so it's probably not an issue, but: Is there any
need to check for malicious input, possibly in the form of very large string
lengths?  I know that this was a concern for the Autofill IPC code; I don't know
whether Mojo has some ... mojo ... to handle this at some higher level of
abstraction.

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits.h:70: // reading
CallStackProfile.
Sorry, I'm not following what this comment means.  Where must this check happen?
 Do you mean that it's somehow happening in the line below?

I *think* that this validation actually happens within ValidateSamples().  But,
I imagine that it's possible to call this function without reaching the
ValidateSamples() code below, right?  If so, it would be nice if this caveat
were more prominently documented somewhere.  Maybe there isn't a better spot for
it, though.  (Have I mentioned that I'm not very fluent in Mojo?)

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits.h:102: if
(frame.module_index >= module_count && frame.module_index !=
nit: This is a weird spot to break the line.  Is this clang-format output?

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
File components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc
(right):

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc:65:
base::MessageLoop message_loop;
nit: Why not declare this as part of the harness?

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc:75:
Module(0x10, "", base::FilePath(base::FilePath::kCurrentDirectory)),
nit: It would be great to have a brief comment for each of the input cases,
documenting what it's intending to test.  (And ditto for similar constructs
elsewhere in this file.)

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc:122:
using Profile = base::StackSamplingProfiler::CallStackProfile;
Hmm, why not write these using statements at the top of the file?

[Mike Wittman, 2016-08-18 17:49:19]

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
File components/metrics/public/cpp/call_stack_profile_struct_traits.h (right):

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits.h:39: if
(!data.ReadId(&id) || !data.ReadFilename(&filename))
On 2016/08/18 07:59:18, Ilya Sherman wrote:
> Tom didn't comment on this, so it's probably not an issue, but: Is there any
> need to check for malicious input, possibly in the form of very large string
> lengths?  I know that this was a concern for the Autofill IPC code; I don't
know
> whether Mojo has some ... mojo ... to handle this at some higher level of
> abstraction.

The only use made of the filename is to generate a hash value, so long strings
there shouldn't be an issue.

The id is used as an opaque value so there shouldn't be an issue there either.
The expected values are at most 40 bytes, however, so I added a check for that.

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits.h:70: // reading
CallStackProfile.
On 2016/08/18 07:59:18, Ilya Sherman wrote:
> Sorry, I'm not following what this comment means.  Where must this check
happen?
>  Do you mean that it's somehow happening in the line below?
> 
> I *think* that this validation actually happens within ValidateSamples(). 
But,
> I imagine that it's possible to call this function without reaching the
> ValidateSamples() code below, right?  If so, it would be nice if this caveat
> were more prominently documented somewhere.  Maybe there isn't a better spot
for
> it, though.  (Have I mentioned that I'm not very fluent in Mojo?)

Clarified the comment.

Frames are not meaningful outside the context of a CallStackProfile, so this
function would never be executed without the check below happening subsequently.

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits.h:102: if
(frame.module_index >= module_count && frame.module_index !=
On 2016/08/18 07:59:18, Ilya Sherman wrote:
> nit: This is a weird spot to break the line.  Is this clang-format output?

Updated formatting to clang output.

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
File components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc
(right):

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc:65:
base::MessageLoop message_loop;
On 2016/08/18 07:59:18, Ilya Sherman wrote:
> nit: Why not declare this as part of the harness?

Done.

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc:75:
Module(0x10, "", base::FilePath(base::FilePath::kCurrentDirectory)),
On 2016/08/18 07:59:18, Ilya Sherman wrote:
> nit: It would be great to have a brief comment for each of the input cases,
> documenting what it's intending to test.  (And ditto for similar constructs
> elsewhere in this file.)

Done.

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc:122:
using Profile = base::StackSamplingProfiler::CallStackProfile;
On 2016/08/18 07:59:18, Ilya Sherman wrote:
> Hmm, why not write these using statements at the top of the file?

The code under test is translating between two sets of similarly-named types, so
I think it's better to keep the type aliases close to the use to avoid potential
confusion.

[Mike Wittman, 2016-08-18 19:09:28]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2016-08-18 19:09:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
isherman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2254743003/#ps100001
(title: "disable test for iOS")

[commit-bot: I haz the power, 2016-08-18 19:09:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2016-08-18 21:39:49]

Thanks, Mike.

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
File components/metrics/public/cpp/call_stack_profile_struct_traits.h (right):

https://codereview.chromium.org/2254743003/diff/60001/components/metrics/publ...
components/metrics/public/cpp/call_stack_profile_struct_traits.h:39: if
(!data.ReadId(&id) || !data.ReadFilename(&filename))
On 2016/08/18 17:49:19, Mike Wittman wrote:
> On 2016/08/18 07:59:18, Ilya Sherman wrote:
> > Tom didn't comment on this, so it's probably not an issue, but: Is there any
> > need to check for malicious input, possibly in the form of very large string
> > lengths?  I know that this was a concern for the Autofill IPC code; I don't
> know
> > whether Mojo has some ... mojo ... to handle this at some higher level of
> > abstraction.
> 
> The only use made of the filename is to generate a hash value, so long strings
> there shouldn't be an issue.

My understanding is that it's undesirable to allow a potentially compromised
remote process result in an attacker controlling a relatively large block of
memory in the browser process, as this makes it easier to circumvent some types
of address randomization defenses.  But, I am not a security expert.

[commit-bot: I haz the power, 2016-08-18 21:56:59]

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

[commit-bot: I haz the power, 2016-08-18 21:57:01]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mike Wittman, 2016-08-18 21:57:47]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2016-08-18 21:58:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 22:53:37]

Description was changed from

==========
Introduce mojo interface for collecting StackSamplingProfiler data

Adds interfaces, typemaps, and tests for collecting
StackSamplingProfiler result types. The collection interface will be
used to receive stack profiles from non-browser processes.

BUG=517958
==========

to

==========
Introduce mojo interface for collecting StackSamplingProfiler data

Adds interfaces, typemaps, and tests for collecting
StackSamplingProfiler result types. The collection interface will be
used to receive stack profiles from non-browser processes.

BUG=517958
==========

[commit-bot: I haz the power, 2016-08-18 22:53:39]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-18 23:24:36]

Description was changed from

==========
Introduce mojo interface for collecting StackSamplingProfiler data

Adds interfaces, typemaps, and tests for collecting
StackSamplingProfiler result types. The collection interface will be
used to receive stack profiles from non-browser processes.

BUG=517958
==========

to

==========
Introduce mojo interface for collecting StackSamplingProfiler data

Adds interfaces, typemaps, and tests for collecting
StackSamplingProfiler result types. The collection interface will be
used to receive stack profiles from non-browser processes.

BUG=517958

Committed: https://crrev.com/551e14b666639639850b2de5588b30b4e40d626b
Cr-Commit-Position: refs/heads/master@{#412963}
==========

[commit-bot: I haz the power, 2016-08-18 23:24:37]

Patchset 6 (id:??) landed as
https://crrev.com/551e14b666639639850b2de5588b30b4e40d626b
Cr-Commit-Position: refs/heads/master@{#412963}

[pauljensen, 2016-08-19 14:34:11]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2264613002/ by pauljensen@chromium.org.

The reason for reverting is: This inadvertently makes skia a dependency of
components/metrics, which nearly doubles the size of Cronet and breaks the
Cronet ARMv6 bot because ARMv6 doesn't support Neon but skia requires Neon..

[chromium-reviews, 2016-08-19 14:45:10]

Drive-by: Probably we should make the stack sampling profiler a separate
target under components/metrics - so that something depending just on
components/metrics (e.g. like Cronet) doesn't need to depend on that target.

Although, it's also surprising to me that a dependency on mojo adds a
dependency on Skia. Probably should file a bug about mojo folks about this.

On Fri, Aug 19, 2016 at 7:34 AM, <pauljensen@chromium.org> wrote:

> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/2264613002/ by pauljensen@chromium.org.
>
> The reason for reverting is: This inadvertently makes skia a dependency of
> components/metrics, which nearly doubles the size of Cronet and breaks the
> Cronet ARMv6 bot because ARMv6 doesn't support Neon but skia requires
> Neon..
>
> https://codereview.chromium.org/2254743003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Wittman, 2016-08-19 16:09:31]

The skia dependency was my doing. A declared dependency on it crept in to one of
the build files at some point during development.

I'll move to a separate target in the reland.

On 2016/08/19 14:45:10, chromium-reviews wrote:
> Drive-by: Probably we should make the stack sampling profiler a separate
> target under components/metrics - so that something depending just on
> components/metrics (e.g. like Cronet) doesn't need to depend on that target.
> 
> Although, it's also surprising to me that a dependency on mojo adds a
> dependency on Skia. Probably should file a bug about mojo folks about this.
> 
> On Fri, Aug 19, 2016 at 7:34 AM, <mailto:pauljensen@chromium.org> wrote:
> 
> > A revert of this CL (patchset #6 id:100001) has been created in
> > https://codereview.chromium.org/2264613002/ by
mailto:pauljensen@chromium.org.
> >
> > The reason for reverting is: This inadvertently makes skia a dependency of
> > components/metrics, which nearly doubles the size of Cronet and breaks the
> > Cronet ARMv6 bot because ARMv6 doesn't support Neon but skia requires
> > Neon..
> >
> > https://codereview.chromium.org/2254743003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.