1289581 - Check all slot assignments for black->gray edges

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: assert_no_slot_sets_gray-v0.diff

I was hoping this wouldn't be needed because of the slowdown, but it looks like we already call out-of-line, so I don't expect any trouble. Other than new assertion failures. This should help us track down what's going off the rails in bug 1289428.

Try run is up at [1], but I haven't even tried this in a browser yet, so who knows how far that will get.

1- https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d38bb14ec92

Comment 1

[Jon Coppeard (:jonco)]

Comment on attachment 8774888 [details] [diff] [review]
assert_no_slot_sets_gray-v0.diff

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

Looks fine.

The non-unified build failure is telling me that you need to include Heap.h though.

Comment 2

[Jon Coppeard (:jonco)]

The original try run didn't seem to catch anything so I pushed a bigger one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79b1c9b74074

Comment 3

[Terrence Cole [:terrence]]

The followup didn't catch anything either, so I pushed a full try run, on top of the other other assertions as well. If the CC-in-JSAPI patch hits but the slot assignment does not, we'll know that something incredibly dangerous is going on.

Comment 4

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=854f68ac8d9e

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #3)
There was this: https://treeherder.mozilla.org/logviewer.html#?job_id=24597359&repo=try#L2197

Comment 6

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b586487773a0

Comment 7

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #5)
> (In reply to Terrence Cole [:terrence] from comment #3)
> There was this:
> https://treeherder.mozilla.org/logviewer.html#?job_id=24597359&repo=try#L2197

Looks like the Promise bug I fixed yesterday.

Comment 8

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a125a7dc496d5366d4065cc58938da5145d41c
Bug 1289581 - Assert that no object slot gets set with a black to gray edge; r=jonco

Comment 9

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/104899f933cb87f49cb7a5e0deb0d4bb952c7700
Bug 1289581 - Follow-up to fix non-unified bustage; r=meow

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d8a125a7dc49
https://hg.mozilla.org/mozilla-central/rev/104899f933cb

Comment 11

[Ryan VanderMeulen [:RyanVM]]

As mentioned in bug 1291776, it would be nice to uplift this to Beta so we can have a consistent story across the branches.

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 8774888 [details] [diff] [review]
assert_no_slot_sets_gray-v0.diff

Approval Request Comment
[Feature/regressing bug #]: The Cycle Collector.
[User impact if declined]: See risks.
[Describe test coverage new/current, TreeHerder]: 2 weeks on TH.
[Risks and why]: None: this adds a single debug-only assertion. The issue it is tracking has been present since ~Firefox 4. Uplifting this will keep our assertion coverage the same between beta, alpha, and trunk, which will allow us to much more easily notice changes in the orange level of the remaining issues so that we know what further fixes need to be uplifted. Taking this patch has zero risk for the release and will potentially let us fix the long-standing issue one release sooner.
[String/UUID change made/needed]: None.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8774888 [details] [diff] [review]
assert_no_slot_sets_gray-v0.diff

Should help debug intermittent test failures, Beta50+

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a0907d658080