Closed Bug 811212 Opened 11 years ago Closed 11 years ago

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


(Core :: XPCOM, defect)

Not set





(Reporter: bjacob, Assigned: bjacob)



(2 files)

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?
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:

                     "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.
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.
Attachment #681106 - Flags: review?(continuation)
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);

So, this is on top of your other patch, so these are the only two NS_ASSERTIONs in the file?
Attachment #681106 - Flags: review?(continuation) → review+
Yes, these are the two only NS_ASSERTIONS in this file.
Attached file updated
try push with all the patches:
Attachment #681197 - Flags: review+
Assignee: nobody → bjacob
Target Milestone: --- → mozilla19
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.