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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

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.

Comment 1

[Darin Fisher]

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.

Comment 2

[Benjamin Smedberg]

dbaron, unless you have a patch together I can do this pretty easily in nsObserverEnumerator::nsObserverEnumerator

Comment 3

[Darin Fisher]

The folks building the observer system for Places (which does not use nsIObserverService) probably want to auto-prune their weak observers as well.

Comment 4

[James Ross]

Just out of interest, what's wrong with the existing observer service, and is their work going to be specific or generic?

Comment 5

[Benjamin Smedberg]

*** Bug 323001 has been marked as a duplicate of this bug. ***

Comment 6

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

Attachment: document the nsIWeakReference contract better

This attempts to document what I believe is the contract provided by the nsIWeakReference / nsISupportsWeakReference interfaces.

Comment 7

[[e-mail]]

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.

Comment 8

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

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.

Comment 9

[Benjamin Smedberg]

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 10

[Benjamin Smedberg]

Comment on attachment 209305 [details] [diff] [review]
document the nsIWeakReference contract better

Please use "[e-mail]" for review requests.

Comment 11

[Darin Fisher]

Comment on attachment 209305 [details] [diff] [review]
document the nsIWeakReference contract better

Please re-request SR once bsmedberg's given r+.

Comment 12

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

Does XBL ever add interfaces as time progresses?

Are we confident that none of the users of aggregation add interfaces over time?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Benjamin Smedberg]

Fixed by bug 326491.