The changes in bug 52510 (checked in 2001-09-27 14:26 PDT) caused the tinderbox leak stats to increase from around 560B (the fix to get them back down to 560B went in very near the same time) to oscillating around 9K.
18 years ago
So, here's the deal, at least partly, I think: The nsHttpHandler registers, in its Init method, as an observer on the pref branch, so that the pref branch owns 4 references to the nsHttpHandler. The nsHttpHandler also holds on to this pref branch as mPrefs. (The pref branch is also owned by the pref service, which is leaked by something else in a bigger cycle..) What's the general rule: should users of the pref service own it, or should they register as listeners? Doing both is prone to leaks -- we've seen this before (e.g., with caps).
w/ out thinking much about it, they should just register as listeners, and not cache the service.
When the observe method is called the associated prefBranch is passed in. This would, in theory, allow the consumer to not maintain a reference to the prefBranch. In practice however most consumers that I have seen also keep the prefBranch around for other preference accesses. The other solution (as in caps) would be to hold a weak reference to the prefBranch. As far as a general rule... That's a tough one. In general, I'd say most consumers just check the preference before using it. There is, however, a certain amount of overhead that goes with this. I would think that it would be more efficient to locally store your own copies of the current state and then listen for changes. This will, unfortunately, add bloat. As a side note, the observer support was added in the nsIPrefBranchInternal interface. If we want to suggest the "general rule" is to observe changes, that support should probably be brought back out to the nsIPrefBranch interface so it is more easily obtainable.
I think a general rule is bad, but there are three techniques we should advertise: 1) keep a weak reference to the branch, check it every time we need the value Best for: whenever we don't need to "react" to a preference changing - i.e. no visual or internal state is transformed because this value changed. An example might be the browser's homepage - when a user changes that pref, it only affects the opening of a new window, so there's no need to "react" to someone changing that preference. 2) keep a callback object which "reacts" to the pref changing. An example of this might be the Print button pref. When the user turns the print button on or off through the preferences, we need all the browser windows to update themselves immediately. 3) Always check, but be lazy if there is no urgency for reacting to the value. An example of this might be the changing of cache size - when the cache size is changed to be smaller, we need to purge disk or memory of some cached pages. However, it may be that the cache doesn't actually get purged until the next page gets read in. In this case, it seems like the simplest solution would be to hold a weak reference to the pref branch.
The reason I suggested a general rule might be needed is that some object A in one component registers as a pref observer, and some object B in another component owns the pref branch or related object, then if A (directly or indirectly) owns a reference to B we end up with a cycle (prefs->A->B->prefs). This would mean practically anytime anyone introduces either type of ownership model they risk introducing a cycle somewhere. However, what I was forgetting is that all of the prefs->A references would either be broken by something other than a destructor or would just be one-time leaks (A would be a service or some other object that lasts the lifetime of the application).
it should be easy enough to make nsHttpHandler just "cache" the prefBranch for the duration of the Observe callback, since it is not really needed (repeatedly) outside of this callback. i would also like it if we added some comments to nsIPrefBranchInternal that just warn consumers to think very carefully about potential cycles. -> moz 0.9.5
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.5
Sure I'm rev'ing the documentation for API freeze anyway. Easily done.
actually, I just remembered that in the pref listener's Observe() method, the nsISupports parameter can actually be QI'ed to the pref service, and to the root pref branch...that might make it a little simpler.
Actually, 1) isn't entirely correct. We should really update the tooltip on the home button ;-)
And, actually, ;) as I noted above... it's the actual nsIPrefBranch that's passed in the nsISupports parameter, not the nsIPrefService. So you can just QI it into the branch that you created, and use it... no messing around required.
Comment on attachment 51613 [details] [diff] [review] v1.0 fixes leaks this patch fixes the leaks, and also makes http observe changes to the pref (now appropriately renamed to network.http.use-cache), which enables/disables http caching.
Comment on attachment 51613 [details] [diff] [review] v1.0 fixes leaks sr=alecf
Attachment #51613 - Flags: superreview+
Comment on attachment 51613 [details] [diff] [review] v1.0 fixes leaks r=gagan
Attachment #51613 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Whiteboard: [tind-mlk] [patch-in-hand] → [tind-mlk]
Hrm. The leak numbers on tinderbox didn't go down.
reopening... i think i have an idea of what the problem actually is: pref service references the http handler as an observer, but the http handler only removes itself as an observer once it is deleted. i suspect that the observer entry in libpref actually references the pref branch, so therein lies the cycle.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
this wasn't a problem when HTTP used the old nsIPref interface since the callback code would only reference a static function pointer and the http handler via a void* reference.
much like it was a solution for nsObserverList, i think the solution for this bug is to make the pref branch hold only weak references to the pref observers (if the pref observer supports weak reference). see nsObserverList.cpp:218
the reason for this is that it otherwise makes every pref observer add extra code to avoid this kind of cycle. with weak references it makes it pref observer code much simpler.
we've discussed this before, and we decided against it. I think part of the issue was that some people had helper-objects whose only reference was libpref and using weak references would make the object get deleted immediately many people objected to the way nsObserverList tries to be "smart" and maintain a weak ref if possible and fallback to a strong ref, because the caller had no control about how it was maintained (i.e. if you implemented weakref for another unrelated reason, you were forced to be held as a weak reference by nsObserverList) Maybe before this interface freezes we ought to change it to: addObserver(in string aDomain, in nsIObserver observer, in boolean holdWeak); so that the caller can decide?
so, i verified using find-leakers.pl that this patch actually fixes the leak. it makes the pref branch code QI for nsISupportsWeakReference in AddObserver... and, if aObserver implements nsISupportsWeakReference, then the pref branch will only weak reference the observer. as i mentioned before, this is what the observer service does... and i think it is the right solution here as well. details: to avoid excessive QI calls, i added a member to PrefCallbackData to record whether or not the stored pObserver is a weak reference or not. then in NotifyObserver, i just NS_STATIC_CAST to the appropriate interface instead of QI'ing again. basically, i've tried to minimize any extra runtime overhead introduced for the non-weak-reference case. however, this means increasing the size of PrefCallbackData by 50%... i think this is the right trade-off.
Status: REOPENED → ASSIGNED
alecf: missed your comments before posting this patch, and after discussing this some with bnesse, i suppose i see why this is not a feasible solution at this time. so, i see two alternate solutions: 1- use a proxy container object for the http handler which holds only a weak reference back to the http handler. 2- make the http handler observe xpcom-shutdown and cleanup then.
again, i verified this patch using find-leakers.pl, and it seems to do the trick.
Comment on attachment 51746 [details] [diff] [review] patch: solves problem using option 2 (observe xpcom-shutdown) awesome. sr=alecf
Attachment #51746 - Flags: superreview+
Comment on attachment 51746 [details] [diff] [review] patch: solves problem using option 2 (observe xpcom-shutdown) got an r=dougt via AIM
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago → 18 years ago
Resolution: --- → FIXED
pls check this into the branch - PDT+, is 52510 is checkin.
You need to log in before you can comment on or make changes to this bug.