811212 - many (all?) cycle collector assertions look like they should be fatal

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

The NS_ASSERTION's in nsCycleCollector.cpp and nsCycleCollectionParticipant.h look like they are good candidates to be replaced by MOZ_ASSERT.

Can you think of any of those that really shouldn't be fatal in debug builds?

Comment 1

[Andrew McCreight [:mccr8]]

Sounds reasonable. Some of the assertions about only passing in CC participants may not be that great, as apparently that's a thing we do. But we could just remove them, maybe. For instance, maybe this one:

        NS_ASSERTION(nsparti,
                     "Don't add objects that don't participate in collection!");

Sometimes they do make sense, so I'm not sure. There could be some in there we do violate, and it isn't too bad, but they generally look serious enough.

The ones in Participant should definitely be converted. I actually did this locally already for my DOM merging patch, because I am frequently violating it, and it indicates a bug.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: make cc assertions fatal, except the "this is not a CC participant" ones

Another one that I don't know about is "suspected a non-scansafe pointer". Should that one be fatal? in the present patch, it is.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 681106 [details] [diff] [review]
make cc assertions fatal, except the "this is not a CC participant" ones

Review of attachment 681106 [details] [diff] [review]:
-----------------------------------------------------------------

Nit: Please fix the indentation on multi-line assertions.

The non-scansafe-pointer looks like it should be fatal, too, as it seems like a fairly basic property. Basically, it checks that we're only using the cycle collected ref count with actual cycle collected things.

Thanks for doing this!  These are mostly benign assertions that have probably not triggered anywhere in years and years, but a few of these are doozies, and making them fatal could have saved us some headache.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ -318,5 @@
>  {
>    static T* Run(void *p)
>    {
>      nsISupports *s = static_cast<nsISupports*>(p);
> -    NS_ASSERTION(NS_CYCLE_COLLECTION_CLASSNAME(T)::CheckForRightISupports(s),

So, this is on top of your other patch, so these are the only two NS_ASSERTIONs in the file?

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Yes, these are the two only NS_ASSERTIONS in this file.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: updated

try push with all the patches: 
https://tbpl.mozilla.org/?tree=Try&rev=868aaf4b7e45

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

green; new try on a larger scale:
https://tbpl.mozilla.org/?tree=Try&rev=933d57bbe63b

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7a4b00ee35b7

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7a4b00ee35b7