3.55 KB, patch
|Details | Diff | Splinter Review|
Preferences, specifically the nsPrefBranch object, needs to support WeakReferences so we can avoid situations where objects holding <strong> references to the nsPrefBranch are also being held as <strong> references by the nsPrefBranch Observer list.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.2
Created attachment 38633 [details] [diff] [review] Patch to add WeakReference support to preferences.
looks great! sr=alecf anyone else here on the CC want to review? it's really easy..
The known problem with this is that you have to addref before calling AddObserver, otherwise it will get deleted. I wonder why is that. I have looked at this example which actually doesn't addref. http://lxr.mozilla.org/seamonkey/source/editor/base/nsEditorParserObserver.cpp
Few more examples, none of them are adding a reference before addObserver. http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookieService.cpp http://lxr.mozilla.org/seamonkey/source/rdf/chrome/src/nsChromeRegistry.cpp#300 any suggestion?
otherwise what gets deleted? what object? you shouldn't have to addref yourself before calling AddObserver
I am not sure abt nsPrefService but when I applied the same patch to nsAutoConfig, it gets deleted in first call to Observe() and I think that's why there is addref and removeref in nsPrefService before and after AddObserve(). bnesse: you may want to test by removing addrefs before addObserve() to see if it gets released.
hrm.. we're talking about two different AddObserve()'s here - there's one in nsIObserverService::AddObserver() - that implementation will try to hold a weak reference, and that's why you got deleted. Then there's nsIPrefBranch::AddObserver() - that one will only try to keep a reference, it won't try to do anything with weak references. but it looks like nsPrefService is doing the right thing, like you said. So when this patch is applied are we seeing any obvious wierdness?
so if nsPrefService is doing the right thing by addrefing before AddObserve(), why the code in other places not required to do so? The links I have posted earlier have similar situation but they are not addrefing before AddObserver(). Particulary, the nsCookieService case. I have no problem against addrefing but I am trying to understand because I want to apply the same patch to nsAutoConfig.
I don't know.. maybe someone already has a reference to them at the time?
Actually, it turns out that it's not necessary anyway. Because it's being called from Init, the refcount is already 1. The incrementing of refcount is really only necessary if this code is executed from the constructor where the refcount would be 0 (as it was in nsIPref.)
Never mind, nsAutoConfig is getting deleted even with refCnt=1 so I think there is something else going on when we derive from nsSupportsWeakReference. For nsPrefService, I think addref before AddObserve is not necessary but leaving it there doesn't harm either. you can clean it later on. r=mitesh
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [r][sr][seeking checkin approval]
Any chance this change could have broken prefs or possibly allowed the prefs service to go away prematurely? Shortly after this checkin, the commercial tree started having lots of flakey problems with prefs. We seem to be seeing JS console errors every time we try to call nsIPref.GetBoolPref in aim and mailnews in the commercial tree. Problem seems to be isolated to release builds too. This seems to be the only prefs code that changed on the 18th. Just curious. Please see Bug #86683 for more details about some of the commercial tree problems we are having related to JS exceptions from reading prefs.
What this patch does is to derive nsPrefService, nsPrefBranch and nsPref from nsSupportsWeakReference to avoid any possible circular reference and hence memory leaking. There may be a possiblity that nsPref is getting deleted being a weakreference earlier than it is supposed to be. Previously, I think it was not at all getting deleted because of a circular reference.
I don't think this would cause such a problem, only because XPCOM should be holding a strong ref to the prefs service. My only thought is that somehow the root pref branch is disappearing or something....but I still thought state was held in the prefservice..
I did verify that this change didn't cause the preferences to be freed arbitrarily. The preferences service stayed alive for the life of the application launch (and yes, it holds a reference to the root branch). As far as I know the only object which might try an hold a weak reference to the preferences is an ObserverList. Any other objects would have to be physically changed to support weak reference holding of preferences objects.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.