If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Assert that gray tracing does not depend on being a marking tracer

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8578870 [details] [diff] [review]
2.1_make_ismarkingtracer_more_accurate-v0.diff

I'm having trouble stabilizing bug 1135985 patch #2 -- misc impossible-to-debug watchdog timeouts in CC tracing during shutdown that only repro on infra in winxp debug builds.

Thus, I'm splitting that patch up and testing it incrementally. This piece adds a single assertion (plus some long-desired refactoring): crash if we check if a thing is a marking tracer while buffering gray roots. The idea here is that we're changing gray buffering from a marking tracer to a callback tracer, so we'd potentially start marking a different set. By inspection I can't see a way we could reach this: this patch just asserts that I am correct.

Try run at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0be4cb46b140
Attachment #8578870 - Flags: review?(jcoppeard)
Comment on attachment 8578870 [details] [diff] [review]
2.1_make_ismarkingtracer_more_accurate-v0.diff

Review of attachment 8578870 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: js/src/gc/Tracer.h
@@ +353,5 @@
>  void
>  SetMarkStackLimit(JSRuntime *rt, size_t limit);
>  
> +// Return true if this trace is happening on behalf of the the marking phase
> +// of the GC.

Comment needs updating to mention gray marking.  And to remove the second 'the'.

@@ +360,5 @@
> +{
> +    return trc->callback == js::GCMarker::GrayCallback;
> +}
> +
> +// Return true if this trace is happening on behalf of the the marking phase

Ditto 'the the'.
Attachment #8578870 - Flags: review?(jcoppeard) → review+
The try build seems to have hit a load of infra issues, so I pushed another one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4085e5abd78
(Assignee)

Comment 3

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/13c8a1cc5ed3
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/701f405c730d
(Assignee)

Comment 5

3 years ago
D'oh! Clicked on the wrong tab when sorting these checkins into bugs, ignore that last comment.
https://hg.mozilla.org/mozilla-central/rev/13c8a1cc5ed3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.