Preferences needs to support weak references

VERIFIED FIXED in mozilla0.9.2

Status

()

Core
Preferences: Backend
P2
major
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: Brian Nesse (gone), Assigned: Brian Nesse (gone))

Tracking

({memory-leak})

Trunk
mozilla0.9.2
memory-leak
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

17 years ago
Blocks: 83902
Status: NEW → ASSIGNED
Keywords: mlk
Priority: -- → P2
Target Milestone: --- → mozilla0.9.2
(Assignee)

Comment 1

17 years ago
Created attachment 38633 [details] [diff] [review]
Patch to add WeakReference support to preferences.

Comment 2

17 years ago
looks great!
sr=alecf

anyone else here on the CC want to review? it's really easy..

Comment 3

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

Comment 5

17 years ago
otherwise what gets deleted? what object? you shouldn't have to addref yourself
before calling AddObserver

Comment 6

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

Comment 7

17 years ago
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?

Comment 8

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

Comment 9

17 years ago
I don't know.. maybe someone already has a reference to them at the time?
(Assignee)

Comment 10

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

Comment 11

17 years ago
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 
(Assignee)

Updated

17 years ago
Whiteboard: [r][sr][seeking checkin approval]

Comment 12

17 years ago
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)
Blocks: 83989
(Assignee)

Comment 13

17 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [r][sr][seeking checkin approval]
Keywords: verifyme

Comment 14

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

Comment 15

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

Comment 16

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

Comment 17

17 years ago
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.
rubberstamping...
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.