Closed Bug 321040 Opened 19 years ago Closed 19 years ago

observer service should remove null weak references from observer lists when enumerating


(Core :: XPCOM, defect)

Not set





(Reporter: dbaron, Assigned: dbaron)



(Keywords: memory-leak, Whiteboard: [patch])


(1 file)

The observer service's capability for having weak references encourages people not to remove their observers.  For observers that fire often and are added often, this can lead to very long observer lists for some observers.  (I've certainly seen long strings of nulled weak references in observer lists; I'm not sure if they're observers that fire often.)

In any case, to avoid having an O(N^2) algorithm, we should remove null weak references from the observer list when enumerating it (in some way that *that* operation isn't O(N^2)).

At least, we should do this if it's true that nsIWeakReference guarantees that an nsIWeakReference instance for which QueryReferent (to a given IID) returns non-null and then null will never return non-null again.  (That's the guarantee needed here; I don't think we can make a stronger one because of the potential for XPCOM aggregation.  It's my understanding that XPCOM allows addition of interfaces but not removal.)  I think we should document this guarantee in the IDL for nsIWeakReference.

That said, we should encourage removal of observers (perhaps by warning in debug builds when we do the removal of nulled weak references) -- not doing so is a leak of the weak reference objects, at least, which could add up in extreme cases.
I agree that we should auto-prune dead weak references, but I'm not sure about the warning.  I think it should be valid to not remove the weak observer.
dbaron, unless you have a patch together I can do this pretty easily in nsObserverEnumerator::nsObserverEnumerator
The folks building the observer system for Places (which does not use nsIObserverService) probably want to auto-prune their weak observers as well.
Just out of interest, what's wrong with the existing observer service, and is their work going to be specific or generic?
*** Bug 323001 has been marked as a duplicate of this bug. ***
This attempts to document what I believe is the contract provided by the nsIWeakReference / nsISupportsWeakReference interfaces.
Attachment #209305 - Flags: superreview?(darin)
Attachment #209305 - Flags: review?(bsmedberg)
Comment on attachment 209305 [details] [diff] [review]
document the nsIWeakReference contract better

Seems to me that this doesn't take tearoffs into account. Also, the contract for nsISupports appears to be stronger i.e. one change from non-null to null.
It's correct that the contract for nsISupports::QueryInterface is stronger:  only one change is allowed, from null to non-null, to allow for tearoffs and aggregation.  But the point of weak references is that QueryReferent will start returning null when the object goes away.  And since QueryInterface lives underneath QueryReferent the null to non-null transition is hidden behind it too, so it has to allow both transitions.
I don't understand the bit about aggregation/tearoffs: aggregation is not allowed to add "new" interfaces to an object (the interface set may not change over time)... what is the situation where nsIWeakReference could return null and then nonnull?
Comment on attachment 209305 [details] [diff] [review]
document the nsIWeakReference contract better

Please use "" for review requests.
Attachment #209305 - Flags: review?(bsmedberg) → review?(benjamin)
Comment on attachment 209305 [details] [diff] [review]
document the nsIWeakReference contract better

Please re-request SR once bsmedberg's given r+.
Attachment #209305 - Flags: superreview?(darin)
Attachment #209305 - Flags: review?(benjamin) → review-
Does XBL ever add interfaces as time progresses?

Are we confident that none of the users of aggregation add interfaces over time?
> Does XBL ever add interfaces as time progresses?

Not per se (in that, the set of interfaces a binding adds is immutable once the binding has been applied), but as things stand it will add interfaces to a node when it's first applied...  At least in XBL1.  What XBL2 will do is not quite decided yet.

Note that JS-implemented stuff could add interfaces as it goes (just by changing what QueryInterface returns); not sure whether that's something we need to support.
Fixed by bug 326491.
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.