Make memory-infra tests register / unregister with thread safety

Make memory-infra tests register / unregister with thread safety

The DCHECK for correct thread at unregistration fires only when tracing
is enabled. As a first step to make this check stricter, fix the tests
to unregister on right thread.
Move the DCHECK that tests for task runner for polling MDP to
registration time.

BUG=643438, 607533
Committed: https://crrev.com/cc4d7737106490e79b40e4bc946405f324c0d834
Cr-Commit-Position: refs/heads/master@{#440239}

[ssid, 2016-12-20 00:52:14]

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

[commit-bot: I haz the power, 2016-12-20 00:52:45]

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-20 00:53:45]

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

[ssid, 2016-12-20 00:53:46]

I've always wanted to do this. This would increase the probability of catching
races for registration. It will not affect production since it's still DCHECK.

[commit-bot: I haz the power, 2016-12-20 01:17:38]

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

[commit-bot: I haz the power, 2016-12-20 01:17: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...)

[ssid, 2016-12-20 01:42:57]

Patchset #1 (id:1) has been deleted

[ssid, 2016-12-20 01:43:16]

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

[commit-bot: I haz the power, 2016-12-20 01:43:44]

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, 2016-12-20 02:18:50]

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

[commit-bot: I haz the power, 2016-12-20 02:18:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ssid, 2016-12-20 03:37:47]

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

[commit-bot: I haz the power, 2016-12-20 03:38:07]

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, 2016-12-20 04:33:10]

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

[commit-bot: I haz the power, 2016-12-20 04:33:12]

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

[Primiano Tucci (use gerrit), 2016-12-21 11:44:06]

On 2016/12/20 00:53:46, ssid wrote:
> I've always wanted to do this. This would increase the probability of catching
> races for registration. It will not affect production since it's still DCHECK.

Conceptually LG, I shouldn't have added that if(!tracing_enabled) before check
in the first place.
The concrete problem now is that we might have cases like this out there.
For instance all these cc_perftests now are failing because they are violating
the threading assumption.
So, before really landing this I'd like to be in a state where we are confident
this is not going to cause pain to everybody and crash here and there when
people try to use debug or dcheck_always_Enabled builds.
Essentially my fear is that if you land this, flakiness will start popping up.
Maybe we need a more graceful path to get to this state, e.g., having some kind
of whitelist to make sure that:
- newly added MDP always get the DCHECK, regardless of tracing.
- Existing ones preserve the current behavior (dcheck only when tracing is
enabled)
and then we'd slowly reduce the whitelist, enforcing DCHECKS on the MDP one by
one, week by week, so at most we cause one flakiness per time.
And once we get to the state where the whielist is empty we just remove it and
get to the state of this CL.
WDYT?

In the meantime OK to start fixing the unittests, so LGTM for that part (but not
for blindly enabling the full dcheck)

[Primiano Tucci (use gerrit), 2016-12-21 11:44:25]

ah forgot a comment in the previous msg

https://codereview.chromium.org/2592543002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (left):

https://codereview.chromium.org/2592543002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:354:
DCHECK(take_mdp_ownership_and_delete_async)
I'd still kjeep the DCHECK here, without the string, just for documentation and
code readability purposes.

[ssid, 2016-12-21 19:53:28]

Description was changed from

==========
Make memory-infra DCHECKs stricter at registration and unregistration

The DCHECK for correct thread at unregistration fires only when tracing
is enabled. Make it crash always so that the bots can catch regressions
better. Move the DCHECK that tests for task runner for polling MDP to
registration time.

BUG=643438,607533
==========

to

==========
Make memory-infra tests register / unregister with thread safety

The DCHECK for correct thread at unregistration fires only when tracing
is enabled. As a first step to make this check stricter, fix the tests
to unregister on right thread.
Move the DCHECK that tests for task runner for polling MDP to
registration time.

BUG=643438,607533
==========

[ssid, 2016-12-21 19:55:03]

Makes sense. I will write another CL with the blacklist for dcheck. Thanks

https://codereview.chromium.org/2592543002/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (left):

https://codereview.chromium.org/2592543002/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:354:
DCHECK(take_mdp_ownership_and_delete_async)
On 2016/12/21 11:44:25, Primiano - Oh Oh Oh till Jan 3 wrote:
> I'd still kjeep the DCHECK here, without the string, just for documentation
and
> code readability purposes.

Done.

[ssid, 2016-12-21 19:55:13]

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

[commit-bot: I haz the power, 2016-12-21 19:56:09]

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, 2016-12-21 21:11:41]

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

[commit-bot: I haz the power, 2016-12-21 21:11:42]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-12-21 22:15:34]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-12-21 22:15:34]

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/2592543002/#ps60001
(title: "Just fix tests.")

[commit-bot: I haz the power, 2016-12-21 22:16:11]

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-12-21 22:22:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482358534363910,
"parent_rev": "6d4e1b85618db4e5eaf14c369d0f0b2bd9e129de", "commit_rev":
"4e6b02602cbd809c4a2c5003b9394e6c078ff812"}

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

Description was changed from

==========
Make memory-infra tests register / unregister with thread safety

The DCHECK for correct thread at unregistration fires only when tracing
is enabled. As a first step to make this check stricter, fix the tests
to unregister on right thread.
Move the DCHECK that tests for task runner for polling MDP to
registration time.

BUG=643438,607533
==========

to

==========
Make memory-infra tests register / unregister with thread safety

The DCHECK for correct thread at unregistration fires only when tracing
is enabled. As a first step to make this check stricter, fix the tests
to unregister on right thread.
Move the DCHECK that tests for task runner for polling MDP to
registration time.

BUG=643438,607533

Review-Url: https://codereview.chromium.org/2592543002
==========

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

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-21 22:25:58]

Description was changed from

==========
Make memory-infra tests register / unregister with thread safety

The DCHECK for correct thread at unregistration fires only when tracing
is enabled. As a first step to make this check stricter, fix the tests
to unregister on right thread.
Move the DCHECK that tests for task runner for polling MDP to
registration time.

BUG=643438,607533

Review-Url: https://codereview.chromium.org/2592543002
==========

to

==========
Make memory-infra tests register / unregister with thread safety

The DCHECK for correct thread at unregistration fires only when tracing
is enabled. As a first step to make this check stricter, fix the tests
to unregister on right thread.
Move the DCHECK that tests for task runner for polling MDP to
registration time.

BUG=643438,607533

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

[commit-bot: I haz the power, 2016-12-21 22:26:00]

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