515714 - nsCycleCollectionParticipant is never used directly, but the subclasses we QI to don't have IIDs

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

Attachment: Patch, still verifying correctness, rev. 1

There are three cycle-collection classes:

nsCycleCollectionParticipant (pure-virtual, has an IID)
nsScriptObjectTracer (virtual is pure, has a non-virtual helper, no IID)
nsXPCOMCycleCollectionParticipant (implements the above two, no IID)

In the cycle collector we QI objects to nsXPCOMCycleCollectionParticipant: because it doesn't have an IID, any object which implements nsCycleCollectionParticipant is returned.

As far as I can tell everyone in our tree actually implements at least nsScriptObjectTracer, but I'm worried that external code may not use nsXPCOMCycleCollectionParticipant and therefore if the collector tries to call ->Trace() on it it will crash.

My initial solution to this is to coalesce all three of these classes into one; I can't see a good reason why they should be separate.

Comment 1

[Benjamin Smedberg]

the nsRefPtr changes here are because the dependent bug doesn't let you do nsCOMPtr<ConcreteClass> as part of its protection

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 399786 [details] [diff] [review]
Patch, still verifying correctness, rev. 1

I'm not too happy about these classes, but this doesn't seem right either. nsXPCOMCycleCollectionParticipant is for nsISupports-derived classes, some of the methods on nsXPCOMCycleCollectionParticipant don't apply to non-nsISupports objects (eg UnmarkPurple). Wouldn't it make more sense to move the IID to nsXPCOMCycleCollectionParticipant?

Comment 3

[Benjamin Smedberg]

I don't know, there's at least a potential behavior change there. Currently http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#1171 will succeed for something which implements nsCycleCollectionParticipant but not nsXPCOMCycleCollectionParticipant. If we move the IID to nsXPCOMCycleCollectionParticipant that will no longer be the case.

The set of classes this affects, according to DXR, is:

    * JSContextParticipant
    * nsNodeInfoManager::cycleCollection
    * nsTimeout::cycleCollection
    * nsTransactionItem::cycleCollection
    * nsXBLBinding::cycleCollection
    * nsXBLInsertionPoint::cycleCollection
    * nsXBLInsertionPointEntry::cycleCollection
    * nsXPConnect
    * nsXULPrototypeNode

At least nsXPConnect seems suspect to me. I'm not sure about the others: most of them don't seem to inherit from nsISupports and so you couldn't QI them anyway.

Comment 4

[Peter Van der Beken [:peterv]]

Even nsXPConnect can't be QI'ed to nsCycleCollectionParticipant, it is the participant for JS objects. I think not being able to QI to nsCycleCollectionParticipant is perfectly fine.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 399786 [details] [diff] [review]
Patch, still verifying correctness, rev. 1

I think we should maintain the separation between these classes.  The two places I see that QI to nsXPCOMCycleCollectionParticipant really only want things that are nsXPCOMCycleCollectionParticipant; if they're ever called on things that aren't, we have a problem.

I think we should just move the IID from nsCycleCollectionParticipant to nsXPCOMCycleCollectionParticipant (and maybe even leave it as the same IID).