[wrapper-tracing] Avoid firing the write barrier after TraceEpilogue

[wrapper-tracing] Avoid firing the write barrier after TraceEpilogue

In-between TraceEpilogue and performCleanup the write barrier used to fire in
the tri-color marking scheme because we eagerly mark but lazily cleanup. This
change avoids marking new headers after TraceEpilogue has been called.

BUG=468240, 676700
Committed: https://crrev.com/3d1bea3cc87f48e6ea569b7050d941b113a55836
Cr-Commit-Position: refs/heads/master@{#441358}

[Michael Lippautz, 2017-01-03 19:25:41]

Description was changed from

==========
[wrapper-tracing] Avoid firing the write barrier after TraceEpilogue

In-between TraceEpilogue and performCleanup the write barrier used to fire in
the tri-color marking scheme because we eagerly mark but lazily cleanup. This
change avoids marking new headers after TraceEpilogue has been called.

BUG=
==========

to

==========
[wrapper-tracing] Avoid firing the write barrier after TraceEpilogue

In-between TraceEpilogue and performCleanup the write barrier used to fire in
the tri-color marking scheme because we eagerly mark but lazily cleanup. This
change avoids marking new headers after TraceEpilogue has been called.

BUG=468240, 676700
==========

[Michael Lippautz, 2017-01-03 19:25:41]

mlippautz@chromium.org changed reviewers:
+ haraken@chromium.org, hlopko@chromium.org

[Michael Lippautz, 2017-01-03 19:32:46]

ptal; this is an attempt to fix issue 676700, a crasher in performCleanup.

In the old marking world where we just pushed everything on the marking deque,
between TraceEpilogue and performCleanup, the write barrier used to fire and
added new WrapperMarkingData to the marking deque. The cleanup method then just
wiped the deque and we were fine. This is not efficient but definitely safe
because no call to AdvanceTracing happens.

In the new world where we eagerly mark the header. We basically continue marking
the wrapper headers (only the markbit) until cleanup is done. This looks
innocent but since we crash in performCleanup when unmarking a header, I suspect
that the C++ object is gone before we call cleanup. Any ideas?

This patch avoids adding the headers by checking m_tracingInProgress and bailing
out when we are not tracing anymore. This should be safe and more efficient, as
we don't mark or add any WrapperMarkingData to the deque anymore.

[Michael Lippautz, 2017-01-03 19:34:47]

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

[commit-bot: I haz the power, 2017-01-03 19:35:13]

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-03 20:28:19]

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

[commit-bot: I haz the power, 2017-01-03 20:28:20]

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...)

[Michael Lippautz, 2017-01-03 21:31:09]

The CQ bit was checked by mlippautz@chromium.org

[haraken, 2017-01-03 21:31:16]

On 2017/01/03 19:32:46, Michael Lippautz wrote:
> ptal; this is an attempt to fix issue 676700, a crasher in performCleanup.
> 
> In the old marking world where we just pushed everything on the marking deque,
> between TraceEpilogue and performCleanup, the write barrier used to fire and
> added new WrapperMarkingData to the marking deque. The cleanup method then
just
> wiped the deque and we were fine. This is not efficient but definitely safe
> because no call to AdvanceTracing happens.
> 
> In the new world where we eagerly mark the header. We basically continue
marking
> the wrapper headers (only the markbit) until cleanup is done. This looks
> innocent but since we crash in performCleanup when unmarking a header, I
suspect
> that the C++ object is gone before we call cleanup. Any ideas?

I don't know how this happens, but this change itself looks reachable. LGTM.

[commit-bot: I haz the power, 2017-01-03 21:32:02]

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-03 23:28:34]

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

[commit-bot: I haz the power, 2017-01-03 23:28:35]

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_...)

[Michael Lippautz, 2017-01-04 08:56:32]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2017-01-04 08:56:32]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2609883003/#ps40001
(title: "Fix tests: Wrap testing code with TracePrologue and AbortTracing")

[commit-bot: I haz the power, 2017-01-04 08:56:45]

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

[Marcel Hlopko, 2017-01-04 09:02:23]

lgtm

[commit-bot: I haz the power, 2017-01-04 12:00:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483520192470230,
"parent_rev": "5ec5d5d3a85ffa3f7eec931f9529487139688cb5", "commit_rev":
"3e8a41aaf75e5ebca93300b9a7c6000c93664563"}

[commit-bot: I haz the power, 2017-01-04 12:01:17]

Description was changed from

==========
[wrapper-tracing] Avoid firing the write barrier after TraceEpilogue

In-between TraceEpilogue and performCleanup the write barrier used to fire in
the tri-color marking scheme because we eagerly mark but lazily cleanup. This
change avoids marking new headers after TraceEpilogue has been called.

BUG=468240, 676700
==========

to

==========
[wrapper-tracing] Avoid firing the write barrier after TraceEpilogue

In-between TraceEpilogue and performCleanup the write barrier used to fire in
the tri-color marking scheme because we eagerly mark but lazily cleanup. This
change avoids marking new headers after TraceEpilogue has been called.

BUG=468240, 676700

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

[commit-bot: I haz the power, 2017-01-04 12:01:18]

Committed patchset #3 (id:40001)

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

Description was changed from

==========
[wrapper-tracing] Avoid firing the write barrier after TraceEpilogue

In-between TraceEpilogue and performCleanup the write barrier used to fire in
the tri-color marking scheme because we eagerly mark but lazily cleanup. This
change avoids marking new headers after TraceEpilogue has been called.

BUG=468240, 676700

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

to

==========
[wrapper-tracing] Avoid firing the write barrier after TraceEpilogue

In-between TraceEpilogue and performCleanup the write barrier used to fire in
the tri-color marking scheme because we eagerly mark but lazily cleanup. This
change avoids marking new headers after TraceEpilogue has been called.

BUG=468240, 676700

Committed: https://crrev.com/3d1bea3cc87f48e6ea569b7050d941b113a55836
Cr-Commit-Position: refs/heads/master@{#441358}
==========

[commit-bot: I haz the power, 2017-01-04 12:03:07]

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