317481 - C API for nsTraceRefcnt

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

I've needed to use nsTraceRefcnt's leak logging ability from C code a number of times, most recently to debug bug 317478 (where I needed to log JS_LockGCThing and JS_UnlockGCThing as reference counting).  This time I decided to take a cleaner approach and actually write a small API to do this.  I'm not sure if it's really something we want to check in -- I'm curious what bsmedberg thinks.

Comment 1

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

Attachment: C API for nsTraceRefcnt

bsmedberg, any opinions on how evil this is?

Comment 2

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

Attachment: example usage (track GC thing lock/unlock)

Comment 3

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

Attachment: example usage (track GC thing lock/unlock)

The previous example for tracking lock/unlock of GC things was missing an NS_LOG_ADDREF in js_NewGCThing for objects created locked.

Comment 4

[Benjamin Smedberg]

Comment on attachment 203987 [details] [diff] [review]
C API for nsTraceRefcnt

Well hrm, I like the idea but it seems to be a bit hackier than we actually need. Why can't we just inline the static nsTraceRefCnt methods directly to their C counterparts (or get rid of them altogether, but I suppose they are used throughout the tree and we don't want to go on a global search-n-replace hunt).

Comment 5

[Benjamin Smedberg]

Comment on attachment 203987 [details] [diff] [review]
C API for nsTraceRefcnt

r- to get this off my queue until questions are addressed.

Comment 6

[Benjamin Smedberg]

New patch is in bug 325229

Comment 7

[Benjamin Smedberg]

FIXED by bug 325229.

Comment 8

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

Not fixed, since the macros still aren't usable.

Comment 9

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

Attachment: C API for nsTraceRefcnt

Note that this change is not just a change in delimiters:

-// nsCOMPtr.h allows these macros to be defined by clients
-// These logging functions require dynamic_cast<void *>, so we don't
-// define these macros if we don't have dynamic_cast.
+/* nsCOMPtr.h allows these macros to be defined by clients
+ * These logging functions require dynamic_cast<void*>, so they don't
+ * do anything useful if we don't have dynamic_cast<void*>. */

since bsmedberg didn't update this comment with his patch.  (I'm not sure I like that change, either, but I suppose it's OK.)

Comment 10

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

Fix checked in to trunk.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 203987 [details] [diff] [review]
C API for nsTraceRefcnt

I realize that a different fix went into the trunk but this patch has actually been very useful to us working on the 1.8 branch (for GC logging - bug 307312). Wondering what folks thought of putting it on the branch only.

Comment 12

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

Comment on attachment 203987 [details] [diff] [review]
C API for nsTraceRefcnt

Minusing (just myself, not on behalf of drivers) since we don't want an API that doesn't match what we've added on the trunk, and also since the patch has review-.