220291 - mixing DEBUG and non-DEBUG leaks due to ~nsCOMPtr_base() hack

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

The fix for bug 77112 causes a mix of DEBUG and non-DEBUG code to leak.  (The
fix made it link, but once the fix applies, it leaks.)  I commented on this
possibility in bug 217009 comment 2, and it was independently discovered as a
real problem (see news://news.mozilla.org:119/bkusf2$[e-mail] ).

I think the easiest way to fix this based on the observation that
NSCAP_FEATURE_FACTOR_DESTRUCTOR and NSCAP_FEATURE_DEBUG_PTR_TYPES are currently
exact opposites, so the ifdef-ing of the destructor was never really needed,
with the exception of the nsCOMPtr<nsISupports> specialization.  I think we
should merge the two macros into something like NSCAP_FEATURE_USE_BASE, and have
nsCOMPtr_base always have a destructor that does releasing.  This will require
some changes to the nsCOMPtr<nsISupports> specialization in addition to the
merging of the macros.

Thoughts?

Comment 1

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

Attachment: possible patch

Comment 2

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

Attachment: possible patch

Comment 3

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

Comment on attachment 132167 [details] [diff] [review]
possible patch

scc, opinions on this?	The part I'm least comfortable with is the change to
the nsCOMPtr<nsISupports> specialization -- perhaps that should have the same
code as the general template, and only conditionally use nsCOMPtr_base?

Comment 4

[cls]

*** Bug 220985 has been marked as a duplicate of this bug. ***

Comment 5

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

Might be good to get this in for 1.6alpha so it gets testing with plugins, etc.

Comment 6

[Scott Collins]

Comment on attachment 132167 [details] [diff] [review]
possible patch

to my eye, this patch matches your prose description of the fix.  Seems like a
reasonable approach, but I think we might want to abandon the idea of having
|nsCOMPtr_base| at all.  We will pay a cost for this in total code size, but it
might be reasonable (we should at least check), and we can still factor the
destructors at least among instances of the same type within each compilation
unit.  This would forever banish our aliasing problem, if I remember and
understand correctly, and the lack of choices will prevent the linking
mismatch.  It may well be worth the (potential, and currently unknown) cost in
code size.

Comment 7

[Scott Collins]

Comment on attachment 132167 [details] [diff] [review]
possible patch

r=scc, modulo the comments above

Comment 8

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

The codesize increase on gcc3 from my aliasing fix makes me think that getting
rid of nsCOMPtr_base wouldn't be good for codesize -- although it would be worth
testing with the templatized methods inlined rather than non-inlined.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 132167 [details] [diff] [review]
possible patch

a=asa (on behalf of drivers) for checkin to 1.6alpha

Comment 10

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

Fix checked in to trunk, 2003-10-22 16:29 -0700.

I plan to further investigate the inlining issues in bug 212082.