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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment)

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

14 years ago
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

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

Comment 3

14 years ago
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

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

Comment 5

14 years ago
*** 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)
Whiteboard: [patch]
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.

Comment 9

14 years ago
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

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

Please use "benjamin@smedbergs.us" for review requests.
Attachment #209305 - Flags: review?(bsmedberg) → review?(benjamin)

Comment 11

14 years ago
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)

Updated

14 years ago
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.

Comment 14

13 years ago
Fixed by bug 326491.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.