102332 - 9K leaks on tinderbox: cycle relating to HTTP handler

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

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

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.

Comment 1

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

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

[Judson Valeski]

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

Comment 3

[Brian Nesse (gone)]

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

[Alec Flett]

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.

Comment 5

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

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

Comment 6

[Darin Fisher]

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

Comment 7

[Brian Nesse (gone)]

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

Comment 8

[Alec Flett]

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

[jag (Peter Annema)]

Actually, 1) isn't entirely correct. We should really update the tooltip on the
home button ;-)

Comment 10

[Brian Nesse (gone)]

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 11

[Darin Fisher]

Attachment: v1.0 fixes leaks

Comment 12

[Darin Fisher]

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

[Alec Flett]

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

sr=alecf

Comment 14

[Gagan]

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

r=gagan

Comment 15

[Darin Fisher]

fixed-on-trunk

Comment 16

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

Hrm.  The leak numbers on tinderbox didn't go down.

Comment 17

[Darin Fisher]

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.

Comment 18

[Darin Fisher]

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.

Comment 19

[Darin Fisher]

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

Comment 20

[Darin Fisher]

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.

Comment 21

[Alec Flett]

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?

Comment 22

[Darin Fisher]

Attachment: patch: make AddObserver use a weak reference if supported

Comment 23

[Darin Fisher]

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.

Comment 24

[Darin Fisher]

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.

Comment 25

[Darin Fisher]

Attachment: patch: solves problem using option 2 (observe xpcom-shutdown)

Comment 26

[Darin Fisher]

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

Comment 27

[Alec Flett]

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

awesome. sr=alecf

Comment 28

[Darin Fisher]

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

got an r=dougt via AIM

Comment 29

[Darin Fisher]

ok.. fixed-on-trunk

Comment 30

[Jaime Rodriguez, Jr.]

pls check this into the branch - PDT+, is 52510 is checkin.