Closed Bug 1352373 Opened 3 years ago Closed 3 years ago

Use relaxed gray marking assertions everywhere on older branches


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- unaffected
firefox55 --- unaffected


(Reporter: jonco, Assigned: jonco)




(1 file)

Bug 1335117 fixed some spurious gray marking assertions by relaxing the condition that we assert on, but missed some assertions.

This was eventually fixed with the last patch in bug 1338623, which also tightened up the assertions.

These bugs landed in FF 54 and the former was backported down to FF 52.

I don't want to backport all of bug 1338623 and its dependents, but it would be good to reduce the number of spurious test failures on older branches.  I'm proposing to backport what would have been the fix for the missed assertions in bug 1335117 if bug 1338623 hadn't superseded it.

This doesn't fix any problems but should make the volume of test failures go down.  If we have evidence that any of the other bugs under bug 1317672 is causing our users problems then we should consider backporting them too.
Here's the patch.  This is similar to the last patch in bug 1338623 except it uses the existing FooIsMarkedGray functions, not the new ones added by that bug.
Attachment #8853364 - Flags: review?(sphink)
Attachment #8853364 - Flags: review?(sphink) → review+
Comment on attachment 8853364 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: Incremental GC
[User impact if declined]: Spurious failures in testing.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Minor change to assertions only, no release-build impact.
[String changes made/needed]: None.
Attachment #8853364 - Flags: approval-mozilla-beta?
Comment on attachment 8853364 [details] [diff] [review]

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Requested to reduce failures in testing.
User impact if declined: Spurious failures in testing.
Fix Landed on Version: 55 but see description.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch:  None.
Attachment #8853364 - Flags: approval-mozilla-esr52?
Comment on attachment 8853364 [details] [diff] [review]

fix some test failures on esr52
Attachment #8853364 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Landed with the wrong bug number in the commit message :\
Comment on attachment 8853364 [details] [diff] [review]

Fix for failing tests, not likely to affect users directly, let's take this for beta 10.
Attachment #8853364 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Closed: 3 years ago
Resolution: --- → FIXED
Setting qe-verify- based on Jon's assessment on manual testing needs (see Comment 2) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.