Closed Bug 1289581 Opened 4 years ago Closed 4 years ago

Check all slot assignments for black->gray edges


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox50 --- fixed
firefox51 --- fixed


(Reporter: terrence, Assigned: terrence)




(1 file)

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.

Attachment #8774888 - Flags: review?(jcoppeard)
Comment on attachment 8774888 [details] [diff] [review]

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.
Attachment #8774888 - Flags: review?(jcoppeard) → review+
The original try run didn't seem to catch anything so I pushed a bigger one:
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.
(In reply to Terrence Cole [:terrence] from comment #3)
There was this:
(In reply to Jon Coppeard (:jonco) from comment #5)
> (In reply to Terrence Cole [:terrence] from comment #3)
> There was this:

Looks like the Promise bug I fixed yesterday.
Bug 1289581 - Assert that no object slot gets set with a black to gray edge; r=jonco
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
As mentioned in bug 1291776, it would be nice to uplift this to Beta so we can have a consistent story across the branches.
Flags: needinfo?(terrence.d.cole)
Comment on attachment 8774888 [details] [diff] [review]

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.
Flags: needinfo?(terrence.d.cole)
Attachment #8774888 - Flags: approval-mozilla-beta?
Comment on attachment 8774888 [details] [diff] [review]

Should help debug intermittent test failures, Beta50+
Attachment #8774888 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.