1352373 - Use relaxed gray marking assertions everywhere on older branches

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1352373-use-gray-assertions-everywhere

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.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8853364 [details] [diff] [review]
bug1352373-use-gray-assertions-everywhere

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.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8853364 [details] [diff] [review]
bug1352373-use-gray-assertions-everywhere

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

Comment 4

[Julien Cristau [:jcristau]]

Comment on attachment 8853364 [details] [diff] [review]
bug1352373-use-gray-assertions-everywhere

fix some test failures on esr52

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Landed with the wrong bug number in the commit message :\
https://hg.mozilla.org/releases/mozilla-esr52/rev/edcafd42dd52

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8853364 [details] [diff] [review]
bug1352373-use-gray-assertions-everywhere

Fix for failing tests, not likely to affect users directly, let's take this for beta 10.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Jon's assessment on manual testing needs (see Comment 2) and the fact that this fix has automated coverage.