209571 - leak regression from bug 200632 landing

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

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

The landing of bug 200632 caused a major leak increase on tinderbox. 
trace-malloc leaks went from 299K to 628K (and nsTraceRefcnt leaks from 388 to
3.73K).

Comment 1

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

According to the nsTraceRefcnt stats, the new leaks are:

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
GlobalWindowImpl                                448          -
nsSystemPrincipal                                60          -
nsDOMClassInfo                                  112          -
NavigatorImpl                                    24          -
nsCodebasePrincipal                              72          -
nsPrefService                                    44          -
nsJSRuntimeServiceImpl                           20          -
nsPrefBranch                                     64          -
nsSharedPrefHandler                              84          -
nsVoidArray                                      40          -
nsEntropyCollector                             1044          -
XPCNativeScriptableInfo                          64          -
nsLocalFile                                     116          -
nsSimpleURI                                       4          -
XPCWrappedNativeProto                           224          -
nsAggregatePrincipal                             88          -
XPCNativeScriptableShared                       608     700.00%
XPCWrappedNative                                308     600.00%
nsXPCComponents                                 176     300.00%
TOTAL                                          3600

which surprises my for cookie changes.

Comment 2

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

My guess is you're leaking pref stuff (perhaps nsIPrefBranch).

Comment 3

[Benjamin Smedberg]

I don't think that the refcnt balancer would really help with leaking JS
wrappers, but I can run a test if you have a particular C++ object that you
think is being leaked.

Comment 4

[[e-mail]]

ack, thanks guys!

looking into it now.

Comment 5

[[e-mail]]

okay, I really have no idea what's going on here.

the only significant change in this landing was the merging of nsICookieService
and nsICookieManager into an xpcom singleton, with the requisite
nsCookieService::GetSingleton() and ::FreeSingleton() methods. I've tested that
part of things before and it looks fine; and I can't see how the prefbranch
would be involved here since that hasn't changed.

I've managed to see the whole cookieservice leaked once or twice now, but I
can't reproduce it consistently at all. it only happens every 10th run or so.

Comment 6

[Alec Flett]

well, this looks like a whole chain of leaks...the root is going to be hard to
find.. but off hand I would guess GlobalWindowImpl

you could also trace one of the leaf leaks like nsPrefBranch and see if you
could figure out which object is holding it that is also leaking.

Comment 7

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

Attachment: patch

This fixes the leak.  The cookie service really shouldn't be holding on to
services in other xpcom components once module destructors start running -- at
that point callers should already have released everything in other modules. 
There's probably some sort of owership chain (via the pref service) that was
keeping JS objects (in the pref service?) rooted past XPConnect's module
destructor.

The solution is to assume that all calls into other modules have to be
completed before module destructors (at which point we *should* be doing
library unloading) start running.

Comment 8

[[e-mail]]

Attachment: patch v2

thanks bz, caillon, and dbaron for the help debugging this - much appreciated!

this patch is basically dbaron's plus a bit of cleanup - it turns out that
nsCookieService::Init can't return failure (we've gradually been making all the
members etc optional), so now we can just fold it into the ctor. the rest is
dbaron's fix (changing ::GetSingleton to addref only once, and removing
::FreeSingleton and altering the modulefac accordingly).

phew!

Comment 9

[Darin Fisher]

Comment on attachment 126278 [details] [diff] [review]
patch v2

sr=darin

Comment 10

[[e-mail]]

Attachment: patch v2.1

oops... I made a mistake with the previous patch - we need a refcount > 0 when
we start adding observers etc, so I shifted the addref call into the ctor.
(added some comments also, per caillon's request).

Comment 11

[[e-mail]]

Comment on attachment 126283 [details] [diff] [review]
patch v2.1

darin/alecf - could I ask you to take a look at this (again)?

Comment 12

[Darin Fisher]

Comment on attachment 126283 [details] [diff] [review]
patch v2.1

nit: use NS_ADDREF_THIS() instead.

sr=darin

Comment 13

[Alec Flett]

Comment on attachment 126283 [details] [diff] [review]
patch v2.1

r=alecf

Comment 14

[[e-mail]]

alecf, bbaetz: there's a comment below you may be interested in.

checked in.

before: Lk:630KB
after:  Lk:300KB

yay!

As a side note, shifting the ::Init() method into the ctor increased codesize by
1.3k on the gcc3.3 and 3.2 tboxen (brad and luna). This is because gcc3.x emits
two constructors (one in-charge, one not-in-charge) and three destructors for
every(?) class, which in this case adds a significant amount to codesize. See
(thx to bz for these links):

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=3187
http://sources.redhat.com/ml/gdb/2002-12/msg00313.html
http://gcc.gnu.org/ml/gcc-bugs/1999-12n/msg00756.html

If desired, I could revert this change, but I think that it's a more general
problem we should be aware of (unless it gets fixed in a future gcc). (This
isn't a problem in gcc2.9x).

Comment 15

[[e-mail]]

ok, so I asked bbaetz about this on IRC, and he pointed me to
http://www.cygwin.com/ml/gdb/2003-04/msg00338.html. a snippet:

>  Daniel> GCC has chosen to implement this by duplicating the function,
>  Daniel> including any user-provided code and any compiler-added code.
>  Daniel> A better implementation would have one copy and labels for
>  Daniel> multiple entry points, on systems where that is supported;
>  Daniel> that's temporarily tabled pending a better description of the
>  Daniel> GCC tree structure to describe multiple entry points.

so this is something that GCC should probably fix, not us. (we may be able to
write a clever script to go through and remove redundant ctors where we know
they're not required, but that sounds really complex and could break things...)

http://gcc.gnu.org/ml/gcc/2003-06/msg01981.html suggests it won't be fixed for
3.3.1, and probably not before 3.4.

Comment 16

[Cathleen]

guys, thanks for getting this fixed quickly :-)