[tracing] Sanitize process memory dumps for background mode

[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, jochen@chromium.org

Committed: https://crrev.com/448e5edbdae14ce8b983726b9d89d248f3d52d23
Cr-Commit-Position: refs/heads/master@{#397918}

[ssid, 2016-05-24 00:38:02]

Patchset #1 (id:1) has been deleted

[ssid, 2016-05-24 00:44:09]

Description was changed from

==========
[tracing] Sanitize dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.

BUG=613198
==========

to

==========
[tracing] Sanitize process memory dumps for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.

BUG=613198
==========

[ssid, 2016-05-24 00:46:28]

This isn't fully done (some test files need to be updated outside base). Please
give initial comments, i will fix the tests after the first parse.

The overhead of checking the dump names comes out to be maximum 0.2 ms per dump,
which is roughly 2.5% increase in existing overhead. So, this should be fine.

[ssid, 2016-05-24 00:47:39]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[ssid, 2016-05-24 00:47:39]

+primiano Missed reviewer on last mail.

This isn't fully done (some test files need to be updated outside base). Please
give initial comments, i will fix the tests after the first parse.

The overhead of checking the dump names comes out to be maximum 0.2 ms per dump,
which is roughly 2.5% increase in existing overhead. So, this should be fine.

[ssid, 2016-05-24 01:00:34]

Patchset #1 (id:20001) has been deleted

[Primiano Tucci (use gerrit), 2016-05-27 17:23:35]

Ok approach makes sense with some comments.
1) General suggestion is: just replace sequence of digits with '#' and don't
worry about 0x (just make the pattern foo/bar/meh_#x#)
2) maybe you could keep a map<string,bool/*is_whitelisted*/> in the session
state so you don't have to repeat the whitelist check over and over every time.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/memory...
File base/trace_event/memory_allocator_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/memory...
base/trace_event/memory_allocator_dump.cc:85:
MemoryDumpLevelOfDetail::BACKGROUND) {
I wonder if we want also a NOTREACHED() here so we can detect if somebody is
wasting time pushing strings that are dropped on the floor in tests.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:53: const char* const
kDumpNameWhitelist[] = {
Ok I think at this point we want a memory_infra_background_whitelist.h/cc file
so we put these constants (and the _for_testing logic) for both MDM (from the
other patch) and this one.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:59: bool
IsNameValidForBackgroundMode(const std::string& name,
s/Valid/Whitelisted/ ... which makes then the comment above superfluos.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:60: const char*
whitelisted_name_for_testing) {
see my comment in the other cl, having to pass this testing arg here is very
weird. Let's use the same approach (null terminated array) and having the
for_testing_ pointer.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:71: if (isxdigit(name[i])) {
instead of having this state machine here, can you do something simpler and say
that: contiguous sequences of digits are collapsed into "#" (or whatever
character)
So you can write your patterns simply as foo/bar/0x#/baz

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:231: return dummy_mad_.get();
maybe NOTREACHED also here to help debugging?

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:246: return it->second.get();
should you just leave this as it is and return nullptr?

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:261: if (level_of_detail_ ==
MemoryDumpLevelOfDetail::BACKGROUND)
I don't think global dumps are a problem right? the guid is a uint64 hardly
contains any pii :)

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:279: if (level_of_detail_ ==
MemoryDumpLevelOfDetail::BACKGROUND)
ditto don't think you need this

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:410: if (level_of_detail_ ==
MemoryDumpLevelOfDetail::BACKGROUND)
ditto

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:71: MemoryDumpLevelOfDetail
level_of_detail);
Oh right now you need the args here, makes sense.
Can you just pass all the MemoryDumpArgs instead of just level_of_detail, just
for sake of consistency?

It's going to be a bit duplicated because now we pass the args both as arugment
to OnMemoryDump(args, pmd) and within PMD but ...whatever. It is still
conceptually clean and I don't see a better solution.
In theory we could refactor all the existing dump providers to take the args
form the pmd() and remove the extra argument to OnMemoryDump but... let's not go
there it's not worth it.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:214: std::unique_ptr<MemoryAllocatorDump>
dummy_mad_;
dummy feels it is for test, it's not the case.
Maybe, dunno, noop_mad_, or black_hole_mad_ (inspired from
https://en.wikipedia.org/wiki/Black_hole_(networking) )

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:218: const char*
whitelisted_name_for_testing_;
can we move this to the memory_infra_background_whitelist.h file (see other
comment)

[ssid, 2016-05-27 17:42:48]

Please see few replies inline.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:71: if (isxdigit(name[i])) {
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> instead of having this state machine here, can you do something simpler and
say
> that: contiguous sequences of digits are collapsed into "#" (or whatever
> character)
> So you can write your patterns simply as foo/bar/0x#/baz

Sorry, I don't get it.
Yeah I would still need this state machine here. This is to identify the hex
numbers, which have letters form normal numbers. If I am in hex mode I would
have to skip "abcdef" if not I should include characters. This is even if I
replace the number with #.
Also if this is the case:
Strings with hex will look like:
"foo/bar_0x#/baz"
without will look like:
"foo/bar_#/baz"

Yes with a # the whitelist would looks better. Are you sure we need the #?

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:246: return it->second.get();
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> should you just leave this as it is and return nullptr?

I can't do that. Some dump providers assume that the dump is created because
they created from previous func and they just do a get and use it. But, if we
created a dummy one, we will crash if we return null. This is safer on the field

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:261: if (level_of_detail_ ==
MemoryDumpLevelOfDetail::BACKGROUND)
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> I don't think global dumps are a problem right? the guid is a uint64 hardly
> contains any pii :)

Global dumps are not required. Anyway we report just the totals, it just makes
the trace bigger with no information.
Is there any specific reason we should have them?

Also the global dumps create dumps with hex numbers and they dont use 0x. Makes
it hard to parse them. I would have to add a state that if "0x" or "__" then we
are in hex mode.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:410: if (level_of_detail_ ==
MemoryDumpLevelOfDetail::BACKGROUND)
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> ditto

Why create unnecessary dumps when we are not interested in them in background
mode?

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:71: MemoryDumpLevelOfDetail
level_of_detail);
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> Oh right now you need the args here, makes sense.
> Can you just pass all the MemoryDumpArgs instead of just level_of_detail, just
> for sake of consistency?
> 
> It's going to be a bit duplicated because now we pass the args both as
arugment
> to OnMemoryDump(args, pmd) and within PMD but ...whatever. It is still
> conceptually clean and I don't see a better solution.
> In theory we could refactor all the existing dump providers to take the args
> form the pmd() and remove the extra argument to OnMemoryDump but... let's not
go
> there it's not worth it.

Yes I was thinking the same. Removing the args from OnMemoryDump is a big
change.

Having MemoryDumpArgs here means that I would have to include
memory_dump_provider.h in this file. But that is a wrong include.
memory_dump_provider should depend on process_memory_dump. Not the other way
around.
So, I would have to move MemoryDumpArgs from memory_dump_provider into
process_memory_dump.h. Which will also mean that now provider should actually
include process_memory_dump.
Does that sound good?

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:218: const char*
whitelisted_name_for_testing_;
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> can we move this to the memory_infra_background_whitelist.h file (see other
> comment)

Okay I will make this change in this CL, while the first one still uses a local
list.

[Primiano Tucci (use gerrit), 2016-05-27 17:51:19]

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:71: if (isxdigit(name[i])) {
On 2016/05/27 17:42:48, ssid wrote:
> On 2016/05/27 17:23:35, Primiano Tucci wrote:
> > instead of having this state machine here, can you do something simpler and
> say
> > that: contiguous sequences of digits are collapsed into "#" (or whatever
> > character)
> > So you can write your patterns simply as foo/bar/0x#/baz
> 
> Sorry, I don't get it.
> Yeah I would still need this state machine here. This is to identify the hex
> numbers, which have letters form normal numbers. If I am in hex mode I would
> have to skip "abcdef" if not I should include characters. This is even if I
> replace the number with #.
> Also if this is the case:
> Strings with hex will look like:
> "foo/bar_0x#/baz"
> without will look like:
> "foo/bar_#/baz"
> 
> Yes with a # the whitelist would looks better. Are you sure we need the #?

Ok sorry this was a stupid comment from my side. I forgot that hex digits
contain also letter a-f :)
Ignore my comment. This makes sense.
Just not sure on the isalpha part below. Why stripping hex is not enough?

[Dmitry Skiba, 2016-05-31 06:48:45]

dskiba@google.com changed reviewers:
+ dskiba@google.com

[Dmitry Skiba, 2016-05-31 06:48:46]

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:63: // Remove special characters,
numbers (including hexadecimal which are makred
*marked

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:66: std::unique_ptr<char[]>
stripped_str(new char[length]);
Uhm, why not std::string? And push_back() instead of stripped_str[str_index++],
== instead of strcmp() == 0.

[ssid, 2016-05-31 22:28:28]

Patchset #3 (id:80001) has been deleted

[ssid, 2016-05-31 22:29:09]

Patchset #3 (id:100001) has been deleted

[ssid, 2016-05-31 22:33:20]

Sorry for the delay in reply, it was us holiday.

replies inline. PTAL Thanks.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/memory...
File base/trace_event/memory_allocator_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/memory...
base/trace_event/memory_allocator_dump.cc:85:
MemoryDumpLevelOfDetail::BACKGROUND) {
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> I wonder if we want also a NOTREACHED() here so we can detect if somebody is
> wasting time pushing strings that are dropped on the floor in tests.

Done.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:53: const char* const
kDumpNameWhitelist[] = {
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> Ok I think at this point we want a memory_infra_background_whitelist.h/cc file
> so we put these constants (and the _for_testing logic) for both MDM (from the
> other patch) and this one.

Done.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:59: bool
IsNameValidForBackgroundMode(const std::string& name,
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> s/Valid/Whitelisted/ ... which makes then the comment above superfluos.

Acknowledged.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:60: const char*
whitelisted_name_for_testing) {
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> see my comment in the other cl, having to pass this testing arg here is very
> weird. Let's use the same approach (null terminated array) and having the
> for_testing_ pointer.

Done.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:63: // Remove special characters,
numbers (including hexadecimal which are makred
On 2016/05/31 06:48:46, Dmitry Skiba wrote:
> *marked

Done.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:66: std::unique_ptr<char[]>
stripped_str(new char[length]);
On 2016/05/31 06:48:46, Dmitry Skiba wrote:
> Uhm, why not std::string? And push_back() instead of
stripped_str[str_index++],
> == instead of strcmp() == 0.

Done.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:71: if (isxdigit(name[i])) {
On 2016/05/27 17:51:19, Primiano Tucci wrote:
> On 2016/05/27 17:42:48, ssid wrote:
> > On 2016/05/27 17:23:35, Primiano Tucci wrote:
> > > instead of having this state machine here, can you do something simpler
and
> > say
> > > that: contiguous sequences of digits are collapsed into "#" (or whatever
> > > character)
> > > So you can write your patterns simply as foo/bar/0x#/baz
> > 
> > Sorry, I don't get it.
> > Yeah I would still need this state machine here. This is to identify the hex
> > numbers, which have letters form normal numbers. If I am in hex mode I would
> > have to skip "abcdef" if not I should include characters. This is even if I
> > replace the number with #.
> > Also if this is the case:
> > Strings with hex will look like:
> > "foo/bar_0x#/baz"
> > without will look like:
> > "foo/bar_#/baz"
> > 
> > Yes with a # the whitelist would looks better. Are you sure we need the #?
> 
> Ok sorry this was a stupid comment from my side. I forgot that hex digits
> contain also letter a-f :)
> Ignore my comment. This makes sense.
> Just not sure on the isalpha part below. Why stripping hex is not enough?

isalpha is there because there could be cases where name is
"foo/bar__0xab/baz_12"
We would have to write this as "foo/bar__/baz_".
Maybe if you like the strings to look pretty, i will leave the '/' and
alphabets?
So, it will look like "foo/bar/baz".

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:231: return dummy_mad_.get();
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> maybe NOTREACHED also here to help debugging?

I have changed the tests to use AddAllocatorDumpInternal instead of
CreateAllocatorDump and moved the dcheck to create methods. This is because I
can just test the non-insertion of the strings. Also tests that the black hole
dump is returned.
If not, I could create a IsValidDumpName method and just test that one, without
checking the return value being black hole.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:71: MemoryDumpLevelOfDetail
level_of_detail);
On 2016/05/27 17:42:48, ssid wrote:
> On 2016/05/27 17:23:35, Primiano Tucci wrote:
> > Oh right now you need the args here, makes sense.
> > Can you just pass all the MemoryDumpArgs instead of just level_of_detail,
just
> > for sake of consistency?
> > 
> > It's going to be a bit duplicated because now we pass the args both as
> arugment
> > to OnMemoryDump(args, pmd) and within PMD but ...whatever. It is still
> > conceptually clean and I don't see a better solution.
> > In theory we could refactor all the existing dump providers to take the args
> > form the pmd() and remove the extra argument to OnMemoryDump but... let's
not
> go
> > there it's not worth it.
> 
> Yes I was thinking the same. Removing the args from OnMemoryDump is a big
> change.
> 
> Having MemoryDumpArgs here means that I would have to include
> memory_dump_provider.h in this file. But that is a wrong include.
> memory_dump_provider should depend on process_memory_dump. Not the other way
> around.
> So, I would have to move MemoryDumpArgs from memory_dump_provider into
> process_memory_dump.h. Which will also mean that now provider should actually
> include process_memory_dump.
> Does that sound good?

I think this change warrents for removal of the args from OnMemoryDump. Do you
think I sholuld add a TODO here?

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:214: std::unique_ptr<MemoryAllocatorDump>
dummy_mad_;
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> dummy feels it is for test, it's not the case.
> Maybe, dunno, noop_mad_, or black_hole_mad_ (inspired from
> https://en.wikipedia.org/wiki/Black_hole_(networking) )

Done.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:218: const char*
whitelisted_name_for_testing_;
On 2016/05/27 17:23:35, Primiano Tucci wrote:
> can we move this to the memory_infra_background_whitelist.h file (see other
> comment)

Done.

[ssid, 2016-05-31 22:37:37]

> 2) maybe you could keep a map<string,bool/*is_whitelisted*/> in the session
> state so you don't have to repeat the whitelist check over and over every
time.

I am not sure how to go about this. The map is going to keep increasing in size
because the ids can differ in each dump, and we might just end up wasting memory
for the map.
For the initial version we anyway are aiming for a single dump. So, this map
wouldn't be very useful. I have added this to the doc for the next version. Lets
discuss more if this is feasible.

[ssid, 2016-05-31 23:42:15]

Description was changed from

==========
[tracing] Sanitize process memory dumps for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.

BUG=613198
==========

to

==========
[tracing] Sanitize process memory dumps for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
6. Creates new file memory_infra_background_whitelist to manage
   whitelisting for background mode.

BUG=613198
==========

[ssid, 2016-05-31 23:45:08]

Patchset #5 (id:160001) has been deleted

[ssid, 2016-06-01 00:05:25]

Patchset #4 (id:140001) has been deleted

[Primiano Tucci (use gerrit), 2016-06-02 20:24:05]

Ok looks great overall, just some final comments.
You are right about the set in the session state, it's more likely that will end
up being a source of memory leak. Let's keep it as it is.

At this point I suggest you ping oysteine@ and let him review the whitelisting
logic. Not sure how we sould proceed there, if we need a special review or what.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:71: if (isxdigit(name[i])) {
On 2016/05/31 22:33:20, ssid wrote:
> On 2016/05/27 17:51:19, Primiano Tucci wrote:
> > On 2016/05/27 17:42:48, ssid wrote:
> > > On 2016/05/27 17:23:35, Primiano Tucci wrote:
> > > > instead of having this state machine here, can you do something simpler
> and
> > > say
> > > > that: contiguous sequences of digits are collapsed into "#" (or whatever
> > > > character)
> > > > So you can write your patterns simply as foo/bar/0x#/baz
> > > 
> > > Sorry, I don't get it.
> > > Yeah I would still need this state machine here. This is to identify the
hex
> > > numbers, which have letters form normal numbers. If I am in hex mode I
would
> > > have to skip "abcdef" if not I should include characters. This is even if
I
> > > replace the number with #.
> > > Also if this is the case:
> > > Strings with hex will look like:
> > > "foo/bar_0x#/baz"
> > > without will look like:
> > > "foo/bar_#/baz"
> > > 
> > > Yes with a # the whitelist would looks better. Are you sure we need the #?
> > 
> > Ok sorry this was a stupid comment from my side. I forgot that hex digits
> > contain also letter a-f :)
> > Ignore my comment. This makes sense.
> > Just not sure on the isalpha part below. Why stripping hex is not enough?
> 
> isalpha is there because there could be cases where name is
> "foo/bar__0xab/baz_12"
> We would have to write this as "foo/bar__/baz_".
> Maybe if you like the strings to look pretty, i will leave the '/' and
> alphabets?
> So, it will look like "foo/bar/baz".

Ah right, I think it's ok.

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:231: return dummy_mad_.get();
On 2016/05/31 22:33:20, ssid wrote:
> On 2016/05/27 17:23:35, Primiano Tucci wrote:
> > maybe NOTREACHED also here to help debugging?
> 
> I have changed the tests to use AddAllocatorDumpInternal instead of
> CreateAllocatorDump and moved the dcheck to create methods. This is because I
> can just test the non-insertion of the strings. Also tests that the black hole
> dump is returned.
> If not, I could create a IsValidDumpName method and just test that one,
without
> checking the return value being black hole.

You can just check the allocator_dumps().count("name") as the black hole is not
in the map

https://codereview.chromium.org/2006943003/diff/60001/base/trace_event/proces...
base/trace_event/process_memory_dump.cc:246: return it->second.get();
On 2016/05/27 17:42:48, ssid wrote:
> On 2016/05/27 17:23:35, Primiano Tucci wrote:
> > should you just leave this as it is and return nullptr?
> 
> I can't do that. Some dump providers assume that the dump is created because
> they created from previous func and they just do a get and use it. But, if we
> created a dummy one, we will crash if we return null. This is safer on the
field

Acknowledged.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (left):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:19: #include "base/timer/timer.h"
I think these are conflicts with your other CLs right? Make sure your rebase
properly before landing.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
File base/trace_event/memory_dump_provider.h (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_dump_provider.h:11: #include
"base/trace_event/process_memory_dump.h"
hmm I wonder if this can end up pulling too many headers in blink and cause some
weird side effect due to some name collision. Maybe safer if you move MDArgs to
memory_dump_request_args which is a cleaner header.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:16: const char* const*
g_dump_provider_whitelist_for_testing = nullptr;
Probably a bit more maintainable if you:
- Just call this g_dump_provider_whitelist
- instead of nullptr set this to be non-const and = kDumpProviderWhitelist.
- In the set...fortesting you just reset the g_dump_provider_whitelist

So you don't have to remember to do the conditional below

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:31: bool
IsMemoryAllocatorDumpNameWhitelisted(const std::string& mad_name) {
just |name| should be fine here. It's quite funny to read "mad_name", as in
"crazy_name" :)

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.h (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.h:19: const char* const
kDumpProviderWhitelist[] = {
These should be moved to an anonymous namespace in the .cc file. Or if you need
them to be public only declared here.
If you do this every .cc file that includes this header will end up creating a
new symbol for kDumpProviderWhitelist.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.h:27: const char* const
kAllocatorDumpNameWhitelist[] = {
ditto for this.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:160: black_hole_mad_.reset(new
MemoryAllocatorDump("dummy", this));
I'd create this only if we need it. If we do everything right there will be no
need of the black hole, so you can avoid this malloc.
Maybe create an helper GetBlackHoleMad(). Also if you have that you can remove
all these DCHECK_NE and add a NOTREACHED in that.
Also I'd s/"dummy"/"discarded"/ just to give more hints about what this is
doing.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:171: return mad;
you can keep this as it was if you use the helper.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:204: return dump_args_.level_of_detail
== MemoryDumpLevelOfDetail::BACKGROUND
nit: At this point stay consistent and keep doing:
if (... == background)
  return blackhole;
return nullptr.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:363: return;
+ NOTREACHED

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:23: const char* const
kTestDumpNameWhitelist[] = {"WhitelistedTestName", nullptr};
I think you should move this below, because you reference this only in one
TEST_F right?

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:247:
CreateAllocatorDumpInPMD(pmd.get(), "NotWhitelistedTestName"));
Isn't this going to hit all those DCHECKs? I guess you just tested this in
release :)
If you do the GetBlackHole helper, you can add a static bool
black_hole_non_fatal_for_testing to PMD.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:249:
CreateAllocatorDumpInPMD(pmd.get(), "NotWhitelistedTestName"));
I think this is identical to the above?

https://codereview.chromium.org/2006943003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/2006943003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:118: :
m_currentProcessMemoryDump(new base::trace_event::ProcessMemoryDump(nullptr, {
base::trace_event::MemoryDumpLevelOfDetail::DETAILED }))
OH I wonder if for consistency you should reset
m_currentProcessMemoryDump.levelofdetail in the ::OnMemoryDump.

[ssid, 2016-06-03 01:59:47]

Thanks, made most of the suggested changes.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (left):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:19: #include "base/timer/timer.h"
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> I think these are conflicts with your other CLs right? Make sure your rebase
> properly before landing.

Oh I removed this intentionally because this is included in the header file. I
forgot to remove in previous cl.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
File base/trace_event/memory_dump_provider.h (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_dump_provider.h:11: #include
"base/trace_event/process_memory_dump.h"
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> hmm I wonder if this can end up pulling too many headers in blink and cause
some
> weird side effect due to some name collision. Maybe safer if you move MDArgs
to
> memory_dump_request_args which is a cleaner header.

Done.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:16: const char* const*
g_dump_provider_whitelist_for_testing = nullptr;
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> Probably a bit more maintainable if you:
> - Just call this g_dump_provider_whitelist
> - instead of nullptr set this to be non-const and = kDumpProviderWhitelist.
> - In the set...fortesting you just reset the g_dump_provider_whitelist
> 
> So you don't have to remember to do the conditional below

Thanks, done

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:31: bool
IsMemoryAllocatorDumpNameWhitelisted(const std::string& mad_name) {
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> just |name| should be fine here. It's quite funny to read "mad_name", as in
> "crazy_name" :)

haha done.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.h (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.h:19: const char* const
kDumpProviderWhitelist[] = {
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> These should be moved to an anonymous namespace in the .cc file. Or if you
need
> them to be public only declared here.
> If you do this every .cc file that includes this header will end up creating a
> new symbol for kDumpProviderWhitelist.

Sorry, fixed.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.h:27: const char* const
kAllocatorDumpNameWhitelist[] = {
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> ditto for this.

Done.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:160: black_hole_mad_.reset(new
MemoryAllocatorDump("dummy", this));
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> I'd create this only if we need it. If we do everything right there will be no
> need of the black hole, so you can avoid this malloc.
> Maybe create an helper GetBlackHoleMad(). Also if you have that you can remove
> all these DCHECK_NE and add a NOTREACHED in that.
> Also I'd s/"dummy"/"discarded"/ just to give more hints about what this is
> doing.

Added a get method. dcheck_ne was added because of tests, explained below.
Removed anyways.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:171: return mad;
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> you can keep this as it was if you use the helper.

This is what I tried to explain in my previous comment.
I have added this dcheck here only because it makes it possible to add unittest
for the AddInternal method.
Now that there's bool i can remove this.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:204: return dump_args_.level_of_detail
== MemoryDumpLevelOfDetail::BACKGROUND
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> nit: At this point stay consistent and keep doing:
> if (... == background)
>   return blackhole;
> return nullptr.

Done.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:363: return;
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> + NOTREACHED

Hmm, that needs an ugly fix of checking background mode before creating
suballocation in many dump providers. If you do not like silently ignoring this
part, I will just remove this change and create sub allocations.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:23: const char* const
kTestDumpNameWhitelist[] = {"WhitelistedTestName", nullptr};
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> I think you should move this below, because you reference this only in one
> TEST_F right?

Hm it is bad i will be setting a global g_whitelist (in whitelist.h) using a
stack vaiable which vanishes later. It might cause unexpected behaviour in other
tests in future.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:247:
CreateAllocatorDumpInPMD(pmd.get(), "NotWhitelistedTestName"));
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> Isn't this going to hit all those DCHECKs? I guess you just tested this in
> release :)
> If you do the GetBlackHole helper, you can add a static bool
> black_hole_non_fatal_for_testing to PMD.

No, I had to make the test class and call AddInternal just to not hit this
dcheck. That is the reason I made the ProcessMemoryDumpTest class to call the
AddInternal method.
black_hole_non_fatal_for_testing sounds like a exact thing in need! :D

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:249:
CreateAllocatorDumpInPMD(pmd.get(), "NotWhitelistedTestName"));
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> I think this is identical to the above?

fixed.

https://codereview.chromium.org/2006943003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp
(right):

https://codereview.chromium.org/2006943003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:118: :
m_currentProcessMemoryDump(new base::trace_event::ProcessMemoryDump(nullptr, {
base::trace_event::MemoryDumpLevelOfDetail::DETAILED }))
On 2016/06/02 20:24:04, Primiano Tucci wrote:
> OH I wonder if for consistency you should reset
> m_currentProcessMemoryDump.levelofdetail in the ::OnMemoryDump.

This dump is added to trace when detailed dump and this dump is created even
when tracing is not enabled, at every gc.
So, this shouldn't be reset and it should always be detailed.

[ssid, 2016-06-03 02:04:19]

Description was changed from

==========
[tracing] Sanitize process memory dumps for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
6. Creates new file memory_infra_background_whitelist to manage
   whitelisting for background mode.

BUG=613198
==========

to

==========
[tracing] Sanitize dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
6. New whitelist file to handle whitelisting logic.

BUG=613198
==========

[ssid, 2016-06-03 02:06:27]

Description was changed from

==========
[tracing] Sanitize dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
6. New whitelist file to handle whitelisting logic.

BUG=613198
==========

to

==========
[tracing] Sanitize dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
==========

[ssid, 2016-06-03 02:07:39]

Description was changed from

==========
[tracing] Sanitize dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
==========

to

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
==========

[ssid, 2016-06-03 02:08:09]

Patchset #5 (id:200001) has been deleted

[ssid, 2016-06-03 02:12:40]

Patchset #5 (id:220001) has been deleted

[ssid, 2016-06-03 02:15:29]

ssid@chromium.org changed reviewers:
+ oysteine@chromium.org

[ssid, 2016-06-03 02:15:30]

+oysteine Please take a look at the whitelisting and safeguards logic. 
memory_infra_background_whitelist.cc and MemoryAllocatorDump::AddString. These
are the only two cases strings are added in the traces. All others are just
numbers.

[Primiano Tucci (use gerrit), 2016-06-03 16:24:37]

LGTM with final change (realized only later sorry).
Thanks a lot for the work.
Also wait for Oystein. Maybe would be quicker if you send him an email as a
summary of how our stripping logic works, or we have a VC with him.

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/180001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:363: return;
On 2016/06/03 01:59:46, ssid wrote:
> On 2016/06/02 20:24:04, Primiano Tucci wrote:
> > + NOTREACHED
> 
> Hmm, that needs an ugly fix of checking background mode before creating
> suballocation in many dump providers. If you do not like silently ignoring
this
> part, I will just remove this change and create sub allocations.

Ahh I thought you were not expecting suballocation to be created.
It's fine leave it as it is. If you expect some dumpers to still invoke
AddSuballocation, as long as they are not too many (so we don't waste cpu time)
it's ok to keep this silent.

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:64: } else if
(isalpha(name[i])) {
sorry for the late comment, I realized later when looking at the test.
I think here we should keep also '/','-','_',' '
For simplicity I'd say isalpha() || ispunct() || isblank()

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/proce...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:366: // DCHECK is disabled for testing.
I think you can summarize these three lines as:
DCHECK(is_black_hole_non_fatal_for_testing_);

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/proce...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:270:
pmd->CreateAllocatorDump("Whitelisted/TestName/0xA1b2"));
small change, By looking at this test I just realized that you want to propagate
the '/', otherwise the whitelist will get super awkward.
I mean, otherwise you will have to whitelist: mallocfoobarbaz andstufflikethat.

[ssid, 2016-06-04 00:26:58]

Partially fixed. Thanks

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:64: } else if
(isalpha(name[i])) {
On 2016/06/03 16:24:36, Primiano Tucci wrote:
> sorry for the late comment, I realized later when looking at the test.
> I think here we should keep also '/','-','_',' '
> For simplicity I'd say isalpha() || ispunct() || isblank()

I have only allowed '/'. See other comment. Lets fix this once we have the
strings list.

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/proce...
File base/trace_event/process_memory_dump.cc (right):

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/proce...
base/trace_event/process_memory_dump.cc:366: // DCHECK is disabled for testing.
On 2016/06/03 16:24:36, Primiano Tucci wrote:
> I think you can summarize these three lines as:
> DCHECK(is_black_hole_non_fatal_for_testing_);

Done.

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/proce...
File base/trace_event/process_memory_dump_unittest.cc (right):

https://codereview.chromium.org/2006943003/diff/240001/base/trace_event/proce...
base/trace_event/process_memory_dump_unittest.cc:270:
pmd->CreateAllocatorDump("Whitelisted/TestName/0xA1b2"));
On 2016/06/03 16:24:36, Primiano Tucci wrote:
> small change, By looking at this test I just realized that you want to
propagate
> the '/', otherwise the whitelist will get super awkward.
> I mean, otherwise you will have to whitelist: mallocfoobarbaz
andstufflikethat.

There is no way to avoid awkwardness.
Right now if I allow '_' The name:
"v8/isolate_0x12/heap_spaces_3" -> "v/isolate_/heap_spaces_"

And
"v8/isolate/213/name" -> "v/isolate//name"


It is only about what looks better, with trailing '_'s or without word
separations.
I have allowed only '/' and alphabets. I think it is ok to not have '_'. If you
think it looks bad, i will change this along with the cl that adds the lists.

[ssid, 2016-06-04 00:35:57]

ssid@chromium.org changed reviewers:
+ haraken@chromium.org, thakis@chromium.org

[ssid, 2016-06-04 00:35:58]

+thakis for base/build.gn added two files in base/trace_event.

+haraken for tiny changes in Webkit.

[ssid, 2016-06-04 00:36:40]

Description was changed from

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
==========

to

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, reed@google.com
==========

[ssid, 2016-06-04 00:37:15]

Description was changed from

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, reed@google.com
==========

to

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, reed@google.com, jochen@chromium.org
==========

[haraken, 2016-06-04 00:39:23]

WebKit LGTM

[Nico, 2016-06-04 01:26:09]

lgtm

[oystein (OOO til 10th of July), 2016-06-04 01:50:00]

Looks great for a suitably paranoid first approach, thanks!

lgtm!

[ssid, 2016-06-04 08:07:49]

Description was changed from

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, reed@google.com, jochen@chromium.org
==========

to

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, jochen@chromium.org
==========

[ssid, 2016-06-04 08:08:20]

On 2016/06/04 01:50:00, Oystein wrote:
> Looks great for a suitably paranoid first approach, thanks!
> 
> lgtm!

TBR +shess +jochen for minor fixes in unittests.

[ssid, 2016-06-04 08:09:19]

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

[commit-bot: I haz the power, 2016-06-04 08:09:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006943003/280001

[ssid, 2016-06-04 08:10:57]

ssid@chromium.org changed reviewers:
+ jochen@chromium.org, shess@chromium.org

[commit-bot: I haz the power, 2016-06-04 08:17:34]

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

[commit-bot: I haz the power, 2016-06-04 08:17:36]

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

[ssid, 2016-06-04 08:32:23]

Patchset #7 (id:280001) has been deleted

[ssid, 2016-06-04 10:32:59]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-06-04 10:32:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, thakis@chromium.org, oysteine@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2006943003/#ps300001
(title: "Fix stripping.")

[commit-bot: I haz the power, 2016-06-04 10:33:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006943003/300001

[commit-bot: I haz the power, 2016-06-04 12:41:03]

Description was changed from

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, jochen@chromium.org
==========

to

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, jochen@chromium.org
==========

[commit-bot: I haz the power, 2016-06-04 12:41:05]

Committed patchset #7 (id:300001)

[commit-bot: I haz the power, 2016-06-04 12:42:30]

Description was changed from

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, jochen@chromium.org
==========

to

==========
[tracing] Sanitize process memory dump names for background mode

For background mode:
1. ProcessMemoryDump knows the level of detail.
2. It checks for dump name to be present in whitelist. If not then
   returns a dummy mad. The strings are stripped of numbers (ids) and
   checked against a whitelist of dump names.
3. Disable creating new dumps just to mark suballocations.
4. Disable creation of global allocator dumps.
5. Disable string attributes in allocator dumps.
Also creates a new whitelist file to handle whitelisting logic.

BUG=613198
TBR=shess@chromium.org, jochen@chromium.org

Committed: https://crrev.com/448e5edbdae14ce8b983726b9d89d248f3d52d23
Cr-Commit-Position: refs/heads/master@{#397918}
==========

[commit-bot: I haz the power, 2016-06-04 12:42:31]

Patchset 7 (id:??) landed as
https://crrev.com/448e5edbdae14ce8b983726b9d89d248f3d52d23
Cr-Commit-Position: refs/heads/master@{#397918}

[Scott Hess - ex-Googler, 2016-06-04 15:16:58]

sql/ lgtm

[ssid, 2016-09-07 03:42:37]

Patchset #3 (id:120001) has been deleted

[ssid, 2016-09-07 03:42:46]

Patchset #1 (id:40001) has been deleted

[ssid, 2016-09-07 03:42:57]

Patchset #4 (id:260001) has been deleted