Closed Bug 614238 Opened 9 years ago Closed 7 years ago

Cycle collector should yell and scream if you have a class that can't be QId to its own participant

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: khuey, Assigned: mccr8)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 613027 is basically the following scenario.

nsFoo implements cycle collection properly
nsBar, which derives from nsFoo, implements the cycle collection helper class properly but the magic in QueryInterface.

QIing nsBar to a cycle collection participant gets you nsFoo's participant.  This is nasty for two reasons:

1) The base class's participant probably covers all of the leaks that would be really obvious, making this harder to find.
2) The logic in the super class's participant looks (and might even be) correct, but it's never getting called.

I have a patch coming up that makes us NS_RUNTIMEABORT in a debug build when a class implements a participant but doesn't QI to it.
Comment on attachment 492643 [details] [diff] [review]
Check to make sure a class QIs to its own cycle collection participant.  s

Perhaps nsHTMLOutputElement really needs a CC participant?
Attachment #492643 - Flags: superreview?(jst)
Attachment #492643 - Flags: review?(peterv)
Comment on attachment 492643 [details] [diff] [review]
Check to make sure a class QIs to its own cycle collection participant.  s

I don't think that compiles in a non-debug build.
Attachment #492643 - Flags: superreview?(jst)
Attachment #492643 - Flags: review?(peterv)
Attachment #492643 - Flags: review-
Note that there are more uses of NS_CYCLE_COLLECTION_PARTICIPANT_INSTANCE in the tree that weren't converted.
OS: Windows 7 → All
Hardware: x86 → All
Comment on attachment 492975 [details] [diff] [review]
Check to make sure a class QIs to its own cycle collection participant.  s

This version should compile in an opt build.

The NS_CYCLE_COLLECTION_PARTICIPANT_INSTANCE macros that I didn't change are either for classes that don't implement QI or on XPCWrappedNative/XPCWrappedJS, which I figured weren't necessary to change.
Attachment #492975 - Flags: review?(peterv)
> NS_RUNTIMEABORT(#_class " does not QI to it's own CC participant!");

*its

Since this is debug-only code, NS_RUNTIMEABORT and NS_ABORT_IF_FALSE are equivalent. I'd use NS_ABORT_IF_FALSE to make it clearer that we're not crashing opt builds here.

Love the way the message differs depending on the class. My fuzzer will appreciate that.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #492975 - Flags: review?(peterv) → review?(jst)
Comment on attachment 492975 [details] [diff] [review]
Check to make sure a class QIs to its own cycle collection participant.  s

Duh, this request totally fell through the cracks here :(. And given that Andrew now owns this code, and that he would certainly be more qualified to review this code than I ever was, punting this review to him.
Attachment #492975 - Flags: review?(jst) → review?(continuation)
Comment on attachment 492975 [details] [diff] [review]
Check to make sure a class QIs to its own cycle collection participant.  s

This still sounds like a good idea. I don't know if the patch will still apply or not, so I'm going to clear the review flag. If you think it should, Kyle, just reset the review flag. If you don't have time to look into this, just reassign the bug to me and I'll get to it at some point.
Attachment #492975 - Flags: review?(continuation)
Duplicate of this bug: 837182
yoink
Assignee: khuey → continuation
The landscape has changed here a fair amount.  I have a prototype patch that alters CheckForRightISupports to check for the correct QI, but unfortunately it is too strict.

If a class defines its own Traverse, but uses the CanSkip of its parent, which is probably fairly common (eg nsHTMLDocument, which is where I'm seeing this), then when we do the downcast in the CanSkip, then my check fails, because we're using the participant of the parent, even though it is okay because we're doing it for a method of the participant that parent and child agree on.

I don't really feel like trying to figure out how to come up with a looser notion of participant equivalence, so I think I'll spin the check out into a separate check, and only call it in the Traverse.  It is possible that there's a class out there that, say, only overwrites the Trace method but not the Traverse, so we'll hit this again, but I'll not worry about that for now.
I'm going to have to set this aside for a bit, but this was the general approach I was going to pursue:

1. Add a virtual GetRealParticipant method any time we define a CC participant inner class.

2. In TraverseImpl/UnlinkImpl, etc, grab the TraverseImpl/etc. from the RealParticipant and make sure it is the same as the current method.
Depends on: 906272
I started working on this for a bit, and it has turned up its first problem, bug 906272, in an unexpected way: nsScreen has a CC declaration, but no definition, so the hard-coding of the CC participant in the check (like Kyle does in his patch) results in a link error, because no participant is actually defined, or something.
This is really just an updated version of Kyle's patch.  Any time we downcast, which happens at the start of basically all the ISupports participant methods, we invoke a virtual function to get the "real" participant, then compare that to the participant returned by the QI.
Attachment #492975 - Attachment is obsolete: true
Comment on attachment 792399 [details] [diff] [review]
check participant when we downcast

try run: https://tbpl.mozilla.org/?tree=Try&rev=fe03c887b3fc
Attachment #792399 - Flags: review?(bugs)
Attachment #792399 - Flags: review?(bugs) → review+
Unfortunately, try: -b d -p emulator doesn't actually build B2G debug, which I failed to notice, so I missed this.  Hopefully this is the only one, but in case it isn't here's another actual debug try run:

https://tbpl.mozilla.org/?tree=Try&rev=40149d2583cf
Comment on attachment 793174 [details] [diff] [review]
part 2: remove unused CC declaration for StkCommandEvent

This fixes B2G builds.
Attachment #793174 - Flags: review?(bugs)
Attachment #793174 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/cab38cc3b623
https://hg.mozilla.org/mozilla-central/rev/7f2e5a6bcb50
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.