9K leaks on tinderbox: cycle relating to HTTP handler

RESOLVED FIXED in mozilla0.9.5

Status

()

defect
P2
normal
RESOLVED FIXED
18 years ago
3 years ago

People

(Reporter: dbaron, Assigned: darin.moz)

Tracking

({memory-leak})

Trunk
mozilla0.9.5
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tind-mlk])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Keywords: mlk
Whiteboard: [tind-mlk]
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).

Comment 2

18 years ago
w/ out thinking much about it, they should just register as listeners, and not
cache the service.

Comment 3

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

Comment 4

18 years ago
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).
Assignee

Comment 6

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

Comment 7

18 years ago
Sure I'm rev'ing the documentation for API freeze anyway. Easily done.

Comment 8

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

Comment 9

18 years ago
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.
Assignee

Updated

18 years ago
Whiteboard: [tind-mlk] → [tind-mlk] [patch-in-hand]
Assignee

Comment 11

18 years ago
Posted patch v1.0 fixes leaks (obsolete) — — Splinter Review
Assignee

Comment 12

18 years ago
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 13

18 years ago
Comment on attachment 51613 [details] [diff] [review]
v1.0 fixes leaks

sr=alecf
Attachment #51613 - Flags: superreview+

Comment 14

18 years ago
Comment on attachment 51613 [details] [diff] [review]
v1.0 fixes leaks

r=gagan
Attachment #51613 - Flags: review+
Assignee

Comment 15

18 years ago
fixed-on-trunk
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.
Assignee

Comment 17

18 years ago
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 → ---
Assignee

Comment 18

18 years ago
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.
Assignee

Comment 19

18 years ago
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
Assignee

Comment 20

18 years ago
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.
Assignee

Updated

18 years ago
Attachment #51613 - Attachment is obsolete: true

Comment 21

18 years ago
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?
Assignee

Comment 23

18 years ago
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
Assignee

Comment 24

18 years ago
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.
Assignee

Comment 26

18 years ago
again, i verified this patch using find-leakers.pl, and it seems to do the trick.

Comment 27

18 years ago
Comment on attachment 51746 [details] [diff] [review]
patch: solves problem using option 2 (observe xpcom-shutdown)

awesome. sr=alecf
Attachment #51746 - Flags: superreview+

Updated

18 years ago
Attachment #51746 - Flags: review+
Assignee

Comment 28

18 years ago
Comment on attachment 51746 [details] [diff] [review]
patch: solves problem using option 2 (observe xpcom-shutdown)

got an r=dougt via AIM
Assignee

Comment 29

18 years ago
ok.. fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 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.