Closed Bug 515714 Opened 11 years ago Closed 6 years ago

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

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file)

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.
Attachment #399786 - Flags: superreview?(dbaron)
Attachment #399786 - Flags: review?(peterv)
the nsRefPtr changes here are because the dependent bug doesn't let you do nsCOMPtr<ConcreteClass> as part of its protection
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?
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.
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 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).
Attachment #399786 - Flags: superreview?(dbaron) → superreview-
Attachment #399786 - Flags: review?(peterv) → review-
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.