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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Check to make sure a class QIs to its own cycle collection participant. s

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 3

[Peter Van der Beken [: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.

Comment 4

[Peter Van der Beken [:peterv]]

Note that there are more uses of NS_CYCLE_COLLECTION_PARTICIPANT_INSTANCE in the tree that weren't converted.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Check to make sure a class QIs to its own cycle collection participant. s

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 7

[Jesse Ruderman]

> 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.

Comment 8

[Johnny Stenback (: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.

Comment 9

[Andrew McCreight (out until 2026) [:mccr8]]

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.

Comment 11

[Andrew McCreight (out until 2026) [:mccr8]]

yoink

Comment 12

[Andrew McCreight (out until 2026) [:mccr8]]

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.

Comment 13

[Andrew McCreight (out until 2026) [:mccr8]]

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.

Comment 14

[Andrew McCreight (out until 2026) [:mccr8]]

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.

Comment 15

[Andrew McCreight (out until 2026) [:mccr8]]

Attachment: check participant when we downcast

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.

Comment 16

[Andrew McCreight (out until 2026) [:mccr8]]

Comment on attachment 792399 [details] [diff] [review]
check participant when we downcast

try run: https://tbpl.mozilla.org/?tree=Try&rev=fe03c887b3fc

Comment 17

[Andrew McCreight (out until 2026) [:mccr8]]

Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfecdf10dcba

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ddfaadc99278 for b2g debug bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=26788031&tree=Mozilla-Inbound

Comment 19

[Andrew McCreight (out until 2026) [:mccr8]]

Attachment: part 2: remove unused CC declaration for StkCommandEvent

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 20

[Andrew McCreight (out until 2026) [:mccr8]]

Comment on attachment 793174 [details] [diff] [review]
part 2: remove unused CC declaration for StkCommandEvent

This fixes B2G builds.

Comment 21

[Andrew McCreight (out until 2026) [:mccr8]]

Second time's the charm...

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cab38cc3b623
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2e5a6bcb50

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/cab38cc3b623
https://hg.mozilla.org/mozilla-central/rev/7f2e5a6bcb50