Because the cycle collector uses the canonical nsISupports pointer to get the nsCycleCollectorParticipant for an object, it will always get the nsCycleCollectorParticipant for the main object. That means it will be blind to tearoffs, and the accounting of the refcounts for the main object can be wrong. For example if a tearoff is held by two strong refs from object A and B and the main object is held only by the strong ref from the tearoff, the cycle collector will find a real refcount of 1 for the main object and two strong refs coming from A and B to the main object. I can see two ways to fix this. One is to make the tearoffs add their refcount to the main objects' by forwarding AddRef/Release to the main object in addition to updating their own refcount. That means changing all the tearoffs, which isn't very attractive. Another solution would be to use the nsCycleCollectorParticipant to get at a canonical pointer for an object. That would allow the tearoffs to have their own nsCycleCollectorParticipant which we could get through QI (more breaking of XPCOM rules, yippie ;-)). It would add a bunch of virtual calls though. Thoughts?

Having the tearoff forward their strong refs to the main object sounds like a lower-cost solution to me.

That solution only works if the main object knows about the tearoffs (and can thus destroy them when the joint refcount has gone to zero). At the moment, that's not the case for some of our DOM tearoffs, at least. Though it'd be nice if it were (because then QIing to that interface twice would give you the same tearoff object).

That's why I said "in addition to updating their own refcount".

Oh, I missed that. Sure, that would work.

(In reply to comment #2) > At the moment, that's not the case for some of our DOM tearoffs, at least. > Though it'd be nice if it were (because then QIing to that interface twice > would give you the same tearoff object). Is there a bug on this? /be

Not really. It was a design decision to reduce memory usage, and it's valid COM, so...

At the cost of an extra word for a forward pointer, which often defeats the purpose of having the tearoff in the first place.

And since XPConnect will often cache the tearoff and they should almost always be used from script we shouldn't create too many.

Not sure if the macro should go into XPCOM and this needs some commenting.

Comment on attachment 253241 [details] [diff] [review] v1 s/NON_CACHED/UNCACHED/g ? /be

Also had to back out the patch for bug 330526, unneeded now.

I personally prefer this approach, it will also work with more complex tearoff classes (eg nsXPCWrappedJS). It introduces a new IID, when QIed to that IID the object returns an nsISupports pointer for the object that the cycle collector can use to cast to the concrete class. It is not necessarily the canonical nsISupports pointer, but for most objects they are the same. I added Upcast and Downcast functions to the nsCycleCollectorParticipant, this way the exact casting between nsISupports and concrete class is specified in only one spot.

Comment on attachment 253952 [details] [diff] [review] v2 Graydon's away.

Checked in and updated the wiki page (s/NS_INTERFACE_MAP_ENTRY_CYCLE_COLLECTION/NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION/ and added some info about tearoffs/aggregation).