[memory-infra] Make thread check at unregistration stricter for new dump providers

[memory-infra] Make thread check at unregistration stricter for new dump providers

This CL makes the check for unregistering dump providers on right thread
stricter by checking even if tracing is not enabled. This is made
stricter only for new dump providers while the existing ones (except few
which are verified correct) are added to a blacklist which does not hit
this check. In next CLs this blacklist will be cleared not to cause
flakiness on the bots with lots of dump providers failing.

BUG=643438
Review-Url: https://codereview.chromium.org/2592233002
Cr-Commit-Position: refs/heads/master@{#442777}
Committed: https://chromium.googlesource.com/chromium/src/+/e45198cfa72d361281a496ea16e794fe4aac64c7

[ssid, 2016-12-21 22:43:41]

Patchset #1 (id:1) has been deleted

[ssid, 2016-12-21 22:43:51]

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

[commit-bot: I haz the power, 2016-12-21 22:44:15]

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

[ssid, 2016-12-21 22:46:38]

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

[ssid, 2016-12-21 22:46:38]

Not urgent, ptal when you get back.

The dump providers not in the blacklist now are: BlinkGC,
HostSharedBitmapManager, JavaHeap, Malloc, PartitionAlloc, ProcessMemoryMetrics,
WinHeap

[commit-bot: I haz the power, 2016-12-22 00:59:52]

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

[commit-bot: I haz the power, 2016-12-22 00:59:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Primiano Tucci (use gerrit), 2017-01-06 14:00:40]

LGTM with one suggestion

https://codereview.chromium.org/2592233002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2592233002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:51: // unregistration. These providers
could potentially cause crashes on build bots
either "checks on unregistration" or "checks when unregistering".

https://codereview.chromium.org/2592233002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:55: const char*
kStrictThreadCheckBlacklist[] = {
+const (consts are never enough), i.e.
const char* const kStrictThreadCheckBlacklist, so would make the array actually
immutable (As the kNamingNotation suggests). Otherwise you end up with a mutable
array to immutable char pointers.

https://codereview.chromium.org/2592233002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:90: bool
IsDumpproviderBlackListedForStrictThreadCheck(const char* name) {
my suggestion is that you keep the array above and:

1)  add a std::unordered_set<base::StringPiece, base::StringPieceHash> to the
MDM instance.

2) In the MDM ctor you do:
thread_check_blacklist_.insert(std::begin(kArray), std::end(kArray)) (or
something like that, not sure if this is possible in one op with stringpiece)

3) and then below during unregistration, where you call this function you just
do
thread_check_blacklist_.count((*mdp_iter)->name)

[ssid, 2017-01-10 23:58:28]

Patchset #2 (id:40001) has been deleted

[ssid, 2017-01-10 23:59:28]

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

[ssid, 2017-01-10 23:59:38]

fixed, thanks

https://codereview.chromium.org/2592233002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2592233002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:51: // unregistration. These providers
could potentially cause crashes on build bots
On 2017/01/06 14:00:39, Primiano Tucci (back but slow) wrote:
> either "checks on unregistration" or "checks when unregistering".

Done.

https://codereview.chromium.org/2592233002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:55: const char*
kStrictThreadCheckBlacklist[] = {
On 2017/01/06 14:00:39, Primiano Tucci (back but slow) wrote:
> +const (consts are never enough), i.e.
> const char* const kStrictThreadCheckBlacklist, so would make the array
actually
> immutable (As the kNamingNotation suggests). Otherwise you end up with a
mutable
> array to immutable char pointers.

Sorry forgot const here.

https://codereview.chromium.org/2592233002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:90: bool
IsDumpproviderBlackListedForStrictThreadCheck(const char* name) {
On 2017/01/06 14:00:39, Primiano Tucci (back but slow) wrote:
> my suggestion is that you keep the array above and:
> 
> 1)  add a std::unordered_set<base::StringPiece, base::StringPieceHash> to the
> MDM instance.
> 
> 2) In the MDM ctor you do:
> thread_check_blacklist_.insert(std::begin(kArray), std::end(kArray)) (or
> something like that, not sure if this is possible in one op with stringpiece)
> 
> 3) and then below during unregistration, where you call this function you just
> do
> thread_check_blacklist_.count((*mdp_iter)->name)

Done.

[commit-bot: I haz the power, 2017-01-11 00:00:19]

Dry run: 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, 2017-01-11 01:21:13]

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

[commit-bot: I haz the power, 2017-01-11 01:21:14]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-01-11 03:09:22]

The CQ bit was checked by ssid@chromium.org

[ssid, 2017-01-11 03:09:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2592233002/#ps60001
(title: "Use set instead of searching string list")

[ssid, 2017-01-11 03:09:24]

The CQ bit was unchecked by ssid@chromium.org

[ssid, 2017-01-11 03:10:17]

The CQ bit was checked by ssid@chromium.org

[commit-bot: I haz the power, 2017-01-11 03:11:06]

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, 2017-01-11 03:15:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484104217411350,
"parent_rev": "e0dcd98dbb8363f64857345c317da57cde0588b3", "commit_rev":
"e45198cfa72d361281a496ea16e794fe4aac64c7"}

[commit-bot: I haz the power, 2017-01-11 03:16:21]

Description was changed from

==========
[memory-infra] Make thread check at unregistration stricter for new dump
providers

This CL makes the check for unregistering dump providers on right thread
stricter by checking even if tracing is not enabled. This is made
stricter only for new dump providers while the existing ones (except few
which are verified correct) are added to a blacklist which does not hit
this check. In next CLs this blacklist will be cleared not to cause
flakiness on the bots with lots of dump providers failing.

BUG=643438
==========

to

==========
[memory-infra] Make thread check at unregistration stricter for new dump
providers

This CL makes the check for unregistering dump providers on right thread
stricter by checking even if tracing is not enabled. This is made
stricter only for new dump providers while the existing ones (except few
which are verified correct) are added to a blacklist which does not hit
this check. In next CLs this blacklist will be cleared not to cause
flakiness on the bots with lots of dump providers failing.

BUG=643438

Review-Url: https://codereview.chromium.org/2592233002
Cr-Commit-Position: refs/heads/master@{#442777}
Committed:
https://chromium.googlesource.com/chromium/src/+/e45198cfa72d361281a496ea16e7...
==========

[commit-bot: I haz the power, 2017-01-11 03:16:22]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e45198cfa72d361281a496ea16e7...