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

RESOLVED FIXED in mozilla19

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla19
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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:

        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.
(Assignee)

Comment 2

6 years ago
Created attachment 681106 [details] [diff] [review]
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.
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);
> -    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?
Attachment #681106 - Flags: review?(continuation) → review+
(Assignee)

Comment 4

6 years ago
Yes, these are the two only NS_ASSERTIONS in this file.
(Assignee)

Comment 5

6 years ago
Created attachment 681197 [details]
updated

try push with all the patches: 
https://tbpl.mozilla.org/?tree=Try&rev=868aaf4b7e45
Attachment #681197 - Flags: review+
(Assignee)

Comment 6

6 years ago
green; new try on a larger scale:
https://tbpl.mozilla.org/?tree=Try&rev=933d57bbe63b
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/7a4b00ee35b7
Assignee: nobody → bjacob
Target Milestone: --- → mozilla19

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/7a4b00ee35b7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.