83902 - script security manager leaks self and pref service (cycle)

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect, P1)

Description

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

The script security manager creates a strong reference cycle between itself and
the pref service causing both to leak.  In nsScriptSecurityManager::InitPrefs it
gets an owning pointer to the pref service |mPrefService| and then adds itself
as a pref observer which creates an owning reference the other way around.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Having the security manager explicitly release the pref service (set the comptr
to null) in its dtor should fix this, right?

Comment 2

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

How is the dtor going to be called?  (It's released already in the dtor.)

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Need this for 0.9.2

Comment 4

[Mitchell Stoltz (not reading bugmail)]

David, any suggestions? Should I use a weak reference to hold the pref service,
or is that too much trouble?

Comment 5

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

I'm not really sure what a good way to fix this would be.

Comment 6

[Brian Nesse (gone)]

After a quick look, I'm wondering if you're getting a double addref when you
create the service. Once because it's a nsCOMPtr in the object, and once because
you call do_GetService on it. cc'ing alecf 'cause he understands this *much*
better than I :).

The simplest thing to do is probably to not save the service reference at all.
Just get the service into a local nsCOMPtr in InitPrefs, and then do it again in
SavePrincipal.

Comment 7

[Alec Flett]

for my own reference:
http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#214
1

probably the best idea here is to use a weak reference to hold onto the prefs 
branch..then the branch will go away, and release the script security manager...

Comment 8

[Alec Flett]

oh, but no you're not doing a double addref with do_GetService - I think 
do_GetService ONLY works for nsCOMPtr's in fact.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Is PrefBranch causing this circular reference, or only PrefService? If it's only
PrefService, no problem, I don't really need to cache that. Does the holding of
PrefBranch by SecurityManager cause a cycle too?

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Adding nsBranch keyword as these bugs need to get into RTM.

Comment 11

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - use weak refs for all prefs interfaces in nsScriptSecurityManager

Comment 12

[Mitchell Stoltz (not reading bugmail)]

The patch above fixes the memory leak but causes the warning
"nsJSRuntimeService is missing" to fire at nsPrefService.cpp:724, and the
"Runtime mismatch, so leaking context!" message at prefapi.c:422. Brian, could
you help me debug this? The pref service was not being destroyed before. With
this patch, it is being destroyed, but it look as though nsIJSRuntimeService has
already been destroyed when the nsIPrefService dtor asks for it, which causes
that assertion I'm seeing.

Comment 13

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

The "runtime mismatch, so leaking context" message has been around for a while.
 See bug 76288.

Comment 14

[Mitchell Stoltz (not reading bugmail)]

Ah, so the reference cycle was just hiding that context leak?

Comment 15

[Brian Nesse (gone)]

Yes. As dbaron noted, this has been around for a while now. I thought it showed
up whenever you quit the application, but that may have changed now that we've
eliminated a few other leak cycles.

It's not your fault, so don't worry about it. Though this may raise the level of
importance on getting a fix for bug 76288.

Comment 16

[Mitchell Stoltz (not reading bugmail)]

Target is now 0.9.4, Priority P1.

Comment 17

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

I'm not qualified to review the GetBaseURIScheme stuff (in which I 
include the changes at places that call it and the targetURI/aTargetURI
change), but r=dbaron for the rest, with the following comment:

This:
     mPrefBranchWeakRef = getter_AddRefs(NS_GetWeakReference(prefBranch));
is better written as:
     mPrefBranchWeakRef = do_GetWeakReference(prefBranch);


I also tested the patch (after manually patching some parts that patch 
didn't want to patch) and along with the patch in bug 76288 and the
patch in bug 93087 we're not leaking pref stuff anymore.

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Fix checked in.

Comment 19

[bsharma]

Verified on 2001-09-19-03 build on WinNT

Marking verified as per above developer comments.

Changing the QA contact to [e-mail]