1367496 - Add some more release assert checks of mScanInProgress

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Andrew McCreight [:mccr8]]

In bug 1322536, I checked for reentrance in the CC in a few places, but I noticed that there are a few more phases we could check. There's no check at all in CleanupAfterCollection, and we might as well also check in BeginCollection at the top.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1367496 - Add more release asserts to the cycle collector.

A previous bug missed a few places where we could theoretically
reenter the CC.

Review commit: https://reviewboard.mozilla.org/r/142494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142494/

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8870923 [details]
Bug 1367496 - Add more release asserts to the cycle collector.

https://reviewboard.mozilla.org/r/142494/#review146184

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e23de29b6cc
Add more release asserts to the cycle collector. r=smaug

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6e23de29b6cc

Comment 5

[Andrew McCreight [:mccr8]]

I should check to see if we are hitting these asserts, in a day or two. If not, I'll uplift to beta.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8870923 [details]
Bug 1367496 - Add more release asserts to the cycle collector.

Approval Request Comment
[Feature/Bug causing the regression]: old
[User impact if declined]: This is needed to help diagnose crashes we're seeing mostly on beta and release.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: This has been on Nightly for a few days, and we haven't hit the crash.
[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?]: If we hit the crash this patch adds, we're probably going to crash in another less safe way later anyways.
[String changes made/needed]: none

Comment 7

[Julien Cristau [:jcristau]]

Next week is RC week for 54, I'm a bit wary of taking this now rather than letting it ride to beta55 in a couple of weeks, especially since this is an old bug.  Any strong reasons for taking it now anyway?

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8870923 [details]
Bug 1367496 - Add more release asserts to the cycle collector.

No, that's fine.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76404bf85ef7
Check for too-large PNG width. r=tn