Closed Bug 1323241 Opened 3 years ago Closed 3 years ago

Don't report that cells are gray when we don't know


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed


(Reporter: jonco, Assigned: jonco)



(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+])


(2 files)

The gray marking state can become invalid if we OOM while unmarking gray.  In that case we could report things as gray when they are not.  I guess that makes this s-s since the cycle collector may consider them for collection incorrectly.

This patch changes the API functions to just return false if the gray marking state is invalid.

I moved the validity flag from GCRuntime to the shadow runtime so that we could carry on inling the IsMarkedGray checks.  I'm not sure that's essential.
Attachment #8818324 - Flags: review?(sphink)
Attachment #8818324 - Attachment is patch: true
Comment on attachment 8818324 [details] [diff] [review]

Review of attachment 8818324 [details] [diff] [review]:

Finally noticing the review requests buried in my secure bugmail. Sorry for the delay. Looks good to me.
Attachment #8818324 - Flags: review?(sphink) → review+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Should we take this on Beta as well?
Flags: needinfo?(jcoppeard)
Attached patch bug1323241-betaSplinter Review
Approval Request Comment
[Feature/Bug causing the regression]: Since forever.
[User impact if declined]: Possible crashes / spurious assertion 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?]: It's a fairly simple change to report GC things as gray less often.  It's been on m-c since 9th Jan.
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8833269 - Flags: approval-mozilla-beta?
Comment on attachment 8833269 [details] [diff] [review]

gc fix for beta52
Attachment #8833269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.