Last Comment Bug 15824 - bad refcounting in nsCodebasePrincipal
: bad refcounting in nsCodebasePrincipal
Status: VERIFIED WORKSFORME
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows NT
: P3 normal (vote)
: ---
Assigned To: Norris Boyd
: John Unruh
Mentors:
Depends on: 16466
Blocks: 16654
  Show dependency treegraph
 
Reported: 1999-10-07 22:41 PDT by Warren Harris
Modified: 2000-02-21 10:10 PST (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Warren Harris 1999-10-07 22:41:57 PDT
Bloaty: Refcounting and Memory Bloat
Statistics

|<-------Name------>|<--------------References-------------->|<----------------
Objects---------------->|<------Size----->|
                               Rem    Total      Mean       StdDev       Rem
Total      Mean       StdDev Per-Class      Rem
  78 nsCodebasePrincipal        -3      258 (   -1.33 +/-     1.90)        1
5 (    0.56 +/-     0.83)       36       36

Norris,

I suspect that the refcount is going negative on this class because the mRefCnt
field isn't really being used, and the mJSPrincipals.refcount field is instead.
Maybe somewhere along the way, the nsCodebasePrincipal is getting QI'd to
nsISupports and that's incrementing the other mRefCnt(?). I couldn't catch it
in the act.

We really should fix this so that it doesn't get in the way of tracking down
other refcounting problems.

Warren
Comment 1 Norris Boyd 1999-10-08 09:34:59 PDT
Do you have any test case that reproduces this?
Comment 2 Norris Boyd 1999-10-08 11:05:59 PDT
There's only one vtable pointer in the nsCodebasePrincipal object, and it points
to the overridden forms of AddRef and Release. So a QI should never result in an
object whose AddRef and Release affect nsISupports::mRefCnt.
Comment 3 Warren Harris 1999-10-08 11:48:59 PDT
Ok, well I guess that theory's wrong. But I got this by recompiling with BLOATY
defined in nsISupportsUtil.h and visiting www.mozilla.org, www.yahoo.com and
then drilling down into the yahoo site for several pages. I'll try to reproduce
it again later today.
Comment 4 Norris Boyd 1999-10-12 16:50:59 PDT
I just checked in changes to assert if Release or AddRef of a
nsCodebasePrincipal is called with a mRefcnt != 0. We'll see if anyone else can
reproduce this problem.
Comment 5 Warren Harris 1999-10-12 20:33:59 PDT
Well, that's not really what I had in mind when filing this bug. I'd like to
see you fix the bloat statistics to reflect reality so that people won't think
that nsCodebasePrincipals are leaking. The statistics can get off if
the NS_IMPL_ADDREF/RELEASE macros aren't used, or if the
NS_LOG_ADDREF/NS_LOG_RELEASE macros aren't used consistently in hand-written
AddRef/Release implementations. The statistics can also get off if there's a
real leak.

Also, I'm not sure your assertion is useful for the case where the
nsCodebasePrincipal isn't getting released/destroyed.
Comment 6 Norris Boyd 1999-10-14 15:06:59 PDT
I tried turning on the about:bloat code to look at the nsCodebasePrincipal
refcounts. I tried loading www.mozilla.org, then www.yahoo.com, and then going
down several layers deep into yahoo. I never saw any negative refcounts in the
about:bloat page.

I did see leaks of nsDocuments, which are holding refs to nsCodebasePrincipals
and preventing them from being released. I've filed bug 16466 on that. When that
bug is resolved, I'll ensure again that nsCodebasePrincipals are indeed released
after the associated page goes away.
Comment 7 Warren Harris 1999-10-14 16:39:59 PDT
Ok, I can't reproduce this now either. Maybe something else got fixed and
cleared it up. Maybe we should mark it WORKSFORME.
Comment 8 Norris Boyd 1999-10-14 17:12:59 PDT
Done. Thanks for retrying.
Comment 9 John Unruh 2000-02-15 15:38:09 PST
Verified worksforme.
Comment 10 leger 2000-02-21 10:10:17 PST
Bulk moving all Browser Security bugs to new Security: General component.  The 
previous Security component for Browser will be deleted.

Note You need to log in before you can comment on or make changes to this bug.