935778 - RefCounted<> doesn't report leaks to the leak checker

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Matt Woodrow (:mattwoodrow)]

At least in gfx we're using RefCounted/RefPtr for a lot of classes, and not getting any leak checking for these classes.

Having this would have (probably) have prevented bug 887791.

I guess we want to have some #define that is only true for mozilla in-tree builds, and conditionally include the headers we need for this.

Any thoughts?

I don't mind implementing this, just want agreement on how to do it first.

Comment 1

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

Why can't you just use nsRefPtr?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> Any thoughts?
> 
> I don't mind implementing this, just want agreement on how to do it first.

If you're really serious about this, moving some/all of the leak-checking infrastructure into MFBT is the only way I see to do this.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Why do we need to do that?

Surely just conditionally calling MOZ_COUNT_CTOR or NS_IMPL_ADDREF would be a huge improvement over what we have now.

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #1)
> Why can't you just use nsRefPtr?

At least for gfx/2d, we're trying to be able to build standalone (requiring only MFBT, not gecko).

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Why do we need to do that?
> 
> Surely just conditionally calling MOZ_COUNT_CTOR or NS_IMPL_ADDREF would be
> a huge improvement over what we have now.

MOZ_COUNT_CTOR is defined in xpcom:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTraceRefcnt.h#18

I guess you're thinking about some conditional include of xpcom headers in mfbt for this to work?

Comment 6

[Matt Woodrow (:mattwoodrow)]

I am indeed.

I realize that's pretty far from ideal, but it's easy and huge step up from not getting any automated leak checking.

Comment 7

[Nicholas Nethercote [inactive]]

Surely moving code from XPCOM into MFBT and then including it back into XPCOM would be better?

Comment 8

[Matt Woodrow (:mattwoodrow)]

Probably, yes. But I wasn't offering to do that :)

Comment 9

[Catalin Iacob]

Attachment: link-errors-when-copying-nsTraceRefcnfImpl-to-mfbt.txt

I've looked at moving MOZ_COUNT_CTOR/MOZ_COUNT_DTOR to mfbt, thought to myself "hmm, this is using a lot of ns... stuff", tried to copy nsTraceRefcnt.h, nsTraceRefcntImpl.h and nsTraceRefcntImpl.cpp to mfbt and compile them there too see how many dependencies they're missing.

I was expecting the compiler to fail a lot because of missing includes for things like "prenv.h", "nsCRT.h" etc. but it didn't because it finds them in dist/include which mfbt uses. I was surprised by this, I was expecting mfbt to be standalone and not use -I dist/include. 

There were a few headers not found and 4 other compilation errors that I worked around by commenting out the failing lines. When tests in mfbt try to link they fail and we get to see the missing dependencies (see attachment). Each of those would need to be found either by linking to things defining them, bringing them into mfbt transitively or rewriting TraceRefcntImpl.cpp to not make use of them.

Each of these seems hard and seems to defeat mfbt's purpose (as I understand it) of being standalone and small. Is this correct?

Comment 10

[Nicholas Nethercote [inactive]]

> > Why can't you just use nsRefPtr?
> 
> At least for gfx/2d, we're trying to be able to build standalone (requiring
> only MFBT, not gecko).

Why is that?


> Each of these seems hard and seems to defeat mfbt's purpose (as I understand
> it) of being standalone and small. Is this correct?

Yeah. This seems like a hard problem.

Comment 11

[Joshua Cranmer [:jcranmer]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> > Each of these seems hard and seems to defeat mfbt's purpose (as I understand
> > it) of being standalone and small. Is this correct?
> 
> Yeah. This seems like a hard problem.

In a not-unrelated issue, I looked up porting mozilla::Mutex and friends to MFBT and ran into similar issues with the deadlock detector. One possible middle course is to require users to pass in a header file somewhere that provides definitions of the requisite logging functionality and only enable it where we can actually use it and don't use it when we can't enable it. Setting that infrastructure up might be more effort than it is worth, though...

Comment 12

[(no longer active)]

We also have a similar problem in bug 956338 for turning on thread safety assertions for WeakPtr.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So who is going to own this.  If MFBT is not going to reach feature parity then we should not be using this in Gecko-specific code.

Comment 14

[Mike Hommey [:glandium]]

There is more to it than include issues. MFBT is part of libmozglue. On Linux, that lib is statically linked to executables and its symbols are exported from there. On Mac and Windows, it's a separate shared library. On all platforms, it needs to be separate from libxul for various reasons, ranging from js being a shared library to binary components, to executables using its symbols.

Comment 15

[Mike Hommey [:glandium]]

That being said, for things that live in MFBT *headers*, we can most probably live with dependencies on gecko stuff, in a proper #ifdef (MOZILLA_INTERNAL_API, probably, since it essentially means "this is in libxul")

Comment 16

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #15)
> That being said, for things that live in MFBT *headers*, we can most
> probably live with dependencies on gecko stuff, in a proper #ifdef
> (MOZILLA_INTERNAL_API, probably, since it essentially means "this is in
> libxul")

So can we just call NS_LogCOMPtrAddRef and NS_LogCOMPtrRelease based on MOZILLA_INTERNAL_API and get logging working for Gecko consumers?

Comment 17

[Mike Hommey [:glandium]]

Yes, that should work.

Comment 18

[(no longer active)]

I'll give that a shot.

Comment 19

[(no longer active)]

Attachment: Part 1: Make trace-refcount logging functions accept a void* argument; r=froydnj

Comment 20

[(no longer active)]

Attachment: Part 2: Add trace-refcount support to mozilla::RefPtr; r=froydnj

Comment 21

[(no longer active)]

Attachment: Part 3: Add support for constructor/destructor logging to RefCounted objects; r=froydnj

Comment 22

[(no longer active)]

Note that we may not be able to land this as is because the try server may go orange because of the leaks this might uncover.  I'd like to get review first before going through the rest of the pain here.

Comment 23

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

Comment on attachment 8378028 [details] [diff] [review]
Part 2: Add trace-refcount support to mozilla::RefPtr; r=froydnj

You want to use nsTraceRefcnt::LogAddref somewhere; using only LogCOMPtrAddRef is certainly wrong.

I'm also not convinced that you *can* use LogCOMPtrAddRef (etc.); it requires that the objects logged support dynamic_cast<void*>, which requires that hey have a vtable.  It's also substantially less important than having LogAddRef and LogRelease calls -- it's useful for debugging (since it allows ignoring smart-pointer AddRef and Release calls that are balanced), but doesn't contribute to statistics.

Comment 24

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

Comment on attachment 8378030 [details] [diff] [review]
Part 3: Add support for constructor/destructor logging to RefCounted objects; r=froydnj

LogCtor and LogDtor should only be used for non-refcounted objects.  For refcounted objects, LogAddRef and LogRelase should be sufficient.

Comment 25

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

Comment on attachment 8378027 [details] [diff] [review]
Part 1: Make trace-refcount logging functions accept a void* argument; r=froydnj

The dynamic_cast<void*> is critically important; you can't remove it.

It's what allows us to match up refcount instrumentation that lives inside AddRef and Release methods and has a |this| representing the most derived object with the AddRef and Release that happen on an nsCOMPtr<nsIFoo> which have an nsIFoo* that might be a different address (different subobject).

Comment 26

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

Comment on attachment 8378027 [details] [diff] [review]
Part 1: Make trace-refcount logging functions accept a void* argument; r=froydnj

Er, never mind about the removing it; I see you moved it.

So I guess this is actually fine, as long as you adjust the comments in nsXPCOM.h to make it clear that the aObject *must match* the pointer passed in to NS_LogAddRef / NS_LogRelease, even in the presence of base class subobjects.

(I see now that you actually can use this with mozilla::Refcounted, so this is useful.  But my comments on the other patches still stand.)

Comment 27

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

Comment on attachment 8378027 [details] [diff] [review]
Part 1: Make trace-refcount logging functions accept a void* argument; r=froydnj

>diff --git a/xpcom/glue/nsTraceRefcnt.h b/xpcom/glue/nsTraceRefcnt.h
>index b132540..22b0025 100644
>--- a/xpcom/glue/nsTraceRefcnt.h
>+++ b/xpcom/glue/nsTraceRefcnt.h
>@@ -33,37 +33,41 @@ do {                                                          \
> #define MOZ_COUNT_DTOR_INHERITED(_type, _base)                    \
> do {                                                              \
>   NS_LogDtor((void*)this, #_type, sizeof(*this) - sizeof(_base)); \
> } while (0)
> 
> /* 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*>. */
>+#ifdef HAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR
> #define NSCAP_LOG_ASSIGNMENT(_c, _p)                                \
>   if (_p)                                                           \
>     NS_LogCOMPtrAddRef((_c),static_cast<nsISupports*>(_p))
> 
> #define NSCAP_LOG_RELEASE(_c, _p)                                   \
>   if (_p)                                                           \
>     NS_LogCOMPtrRelease((_c), static_cast<nsISupports*>(_p))
>+#endif

Oh, but you now need to change these to call the one that does the dynamic_cast rather than the one that doesn't; you've left the one that doesn't do the dynamic_cast unused.  Otherwise you'll break the nsCOMPtr usage.

Though I think that suggests that maybe some further function renaming is in order.

Comment 28

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

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #23)
> I'm also not convinced that you *can* use LogCOMPtrAddRef (etc.); it
> requires that the objects logged support dynamic_cast<void*>, which requires
> that hey have a vtable.

Ignore this bit; with mozilla::RefCounted you have the pointer-identity of the RefCounted sub-object, so you can and should useit.  But you still have to use it in *addition to LogAddRef/LogRelease.

Comment 29

[(no longer active)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #28)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #23)
> > I'm also not convinced that you *can* use LogCOMPtrAddRef (etc.); it
> > requires that the objects logged support dynamic_cast<void*>, which requires
> > that hey have a vtable.
> 
> Ignore this bit; with mozilla::RefCounted you have the pointer-identity of
> the RefCounted sub-object, so you can and should useit.  But you still have
> to use it in *addition to LogAddRef/LogRelease.

So to summarize, does this mean that I should just stop using NS_LogCtor/Dtor, start using NS_LogAddRef/Release, and address this and comment 27 and I would be good to go?

Comment 30

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #29)
> So to summarize, does this mean that I should just stop using
> NS_LogCtor/Dtor, start using NS_LogAddRef/Release, and address this and
> comment 27 and I would be good to go?

Yes, I think so.  So maybe patch 2 is ok when combined with that, although it feels very odd for it to come in the sequence before the more fundamental part.

(Also, please don't use the NSCAP_* macros outside of nsCOMPtr's implementation.)

(Note that there is a bug that callers using NS_LogAddRef/NS_LogRelease on a refcounted object that is never reference-counted don't get the implicit LogCtor that should have happened, and thus we fail to report the leak.  It might make sense to try to do that differently for this case, and have a variant of the LogAddRef/LogRelease that don't implicitly log Ctor/Dtor, since it's easily doable here (unlike with the XPCOM case).  In the XPCOM case, that didn't matter much in practice since it was very rare for somebody to entirely forget to refcount an object; with mozilla::RefCounted that might be different.)

Comment 31

[(no longer active)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #30)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #29)
> > So to summarize, does this mean that I should just stop using
> > NS_LogCtor/Dtor, start using NS_LogAddRef/Release, and address this and
> > comment 27 and I would be good to go?
> 
> Yes, I think so.  So maybe patch 2 is ok when combined with that, although
> it feels very odd for it to come in the sequence before the more fundamental
> part.
> 
> (Also, please don't use the NSCAP_* macros outside of nsCOMPtr's
> implementation.)

OK, I'll try to do that the next time I have some free time.  (Might not be until the weekend.)

> (Note that there is a bug that callers using NS_LogAddRef/NS_LogRelease on a
> refcounted object that is never reference-counted don't get the implicit
> LogCtor that should have happened, and thus we fail to report the leak.  It
> might make sense to try to do that differently for this case, and have a
> variant of the LogAddRef/LogRelease that don't implicitly log Ctor/Dtor,
> since it's easily doable here (unlike with the XPCOM case).  In the XPCOM
> case, that didn't matter much in practice since it was very rare for
> somebody to entirely forget to refcount an object; with mozilla::RefCounted
> that might be different.)

Hmm, can you please file this as a follow-up bug with more details about where this special casing lives?  I don't know a lot about this code so I'd prefer to do that part separately on a more relaxed schedule.  Thanks.

Comment 32

[(no longer active)]

I found GenericRefCounted in moz2d code which seems to try to bridge a gap between XPCOM style reference counting and RefCounted style reference counting, which is a bit mind bending.  For the purposes of this bug I'm going to assume that class mostly doesn't exist, and when we're done here the gfx folks can figure out how they want to port this work over.  Needinfo-ing bjacob who wrote that class to make sure he sees this comment, but I'm not requesting any info here since I'm not volunteering to do that work.

Comment 33

[(no longer active)]

So we have another problem here.  The type names passed to the logging functions are used as keys to the bloat entry table, so they actually need to be unique.  So I'm going to start landing those name macros gradually as a prerequisite of this work.  r=mattwoodrow/etc.

Comment 34

[(no longer active)]

Attachment: Part 0.1: Export nsDocShell.h because WeakReference<nsDocShell> will require access to the concrete type in order to find nsDocShell::typeName

We unfortunately need to do this, otherwise any .cpp file which either directly or indirectly includes one of these three headers will have to include nsDocShell.h in order to see the complete definition of nsDocShell (so that it can find nsDocShell::typeName.)

Comment 35

[(no longer active)]

Comment on attachment 8379406 [details] [diff] [review]
Part 0.1: Export nsDocShell.h because WeakReference<nsDocShell> will require access to the concrete type in order to find nsDocShell::typeName

Err, this breaks building some of our compiled tests:

 0:05.01 In file included from /Users/ehsan/moz/src/widget/tests/TestAppShellSteadyState.cpp:10:
 0:05.01 In file included from ../../dist/include/nsIDocument.h:27:
 0:05.01 In file included from ../../dist/include/nsDocShell.h:19:
 0:05.01 In file included from ../../dist/include/nsDocLoader.h:21:
 0:05.01 In file included from ../../dist/include/nsString.h:13:
 0:05.01 In file included from ../../dist/include/nsSubstring.h:11:
 0:05.01 In file included from ../../dist/include/nsAString.h:12:
 0:05.01 ../../dist/include/nsStringFwd.h:16:2: error: Internal string headers are not available from external-linkage code.
 0:05.01 #error Internal string headers are not available from external-linkage code.
 0:05.01  ^
 0:05.01 ../../dist/include/nsStringFwd.h:26:7: error: definition of type 'nsAutoString' conflicts with typedef of the same name
 0:05.01 class nsAutoString;
 0:05.02       ^
 0:05.02 ../../dist/include/nsStringAPI.h:1453:18: note: 'nsAutoString' declared here
 0:05.02 typedef nsString nsAutoString;
 0:05.02                  ^

Gotta resort to a MOZILLA_INTERNAL_API hack.  I'm quickly regretting working on this bug... :/

Comment 36

[(no longer active)]

Attachment: Part 0.1: Export nsDocShell.h because WeakReference<nsDocShell> will require access to the concrete type in order to find nsDocShell::typeName; r=bzbarsky

Comment 37

[(no longer active)]

Attachment: Part 0.1: Export nsDocShell.h because WeakReference<nsDocShell> will require access to the concrete type in order to find nsDocShell::typeName; r=bzbarsky

Comment 38

[(no longer active)]

Comment on attachment 8379416 [details] [diff] [review]
Part 0.1: Export nsDocShell.h because WeakReference<nsDocShell> will require access to the concrete type in order to find nsDocShell::typeName; r=bzbarsky

Sorry Boris, this is not the best patch I've ever written...  But it's much better than having to spray #include "nsDocShell.h" everywhere and add /docshel/base to LOCAL_INCLDUES.  The good news is that nsDocShell.h seems to be the only header we have to export like this...

Comment 39

[(no longer active)]

Spray some MOZ_DECLARE_REFCOUNTED_TYPENAME across the tree:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a29b5994e73a

Comment 40

[(no longer active)]

Attachment: Part 1: Make trace-refcount logging functions accept a void* argument; r=dbaron

Comment 41

[(no longer active)]

Attachment: Part 2: Add RefCountType, a type compatible with nsrefcnt, to MFBT; r=dbaron

Comment 42

[(no longer active)]

Attachment: Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

Comment 43

[(no longer active)]

Attachment: Part 4: Add trace-refcount support to mozilla::RefPtr; r=dbaron

Comment 44

[(no longer active)]

Comment on attachment 8379422 [details] [diff] [review]
Part 1: Make trace-refcount logging functions accept a void* argument; r=dbaron

Review of attachment 8379422 [details] [diff] [review]:
-----------------------------------------------------------------

This is ready for review now.

Comment 45

[(no longer active)]

Comment on attachment 8379423 [details] [diff] [review]
Part 2: Add RefCountType, a type compatible with nsrefcnt, to MFBT; r=dbaron

Review of attachment 8379423 [details] [diff] [review]:
-----------------------------------------------------------------

So is this one.

Comment 46

[(no longer active)]

Comment on attachment 8379424 [details] [diff] [review]
Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

Review of attachment 8379424 [details] [diff] [review]:
-----------------------------------------------------------------

These are mostly ready but not quite.

Comment 47

[(no longer active)]

Please note that I'm planning to remove the RefCounted::typeName() function once I'm done landing these spraying patches, to make it a compile time error to forget to add one of these macros where it's needed.

Comment 48

[(no longer active)]

Spray some more: https://hg.mozilla.org/integration/mozilla-inbound/rev/60c0dbc3c8c5

Comment 49

[Boris Zbarsky [:bzbarsky]]

Why do we need a nsDocShell::typeName for a WeakReference<nsDocShell>, exactly?

Comment 50

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

Comment on attachment 8379422 [details] [diff] [review]
Part 1: Make trace-refcount logging functions accept a void* argument; r=dbaron

So one thing here is that you're patching a bunch of code that's completely unused (nsTraceRefcnt::Log*, which we should have removed ages ago, and nsTraceRefcntImpl::Log*, which we should have removed when we unfroze XPCOM glue binary compatibility, which, ages ago, used nsITraceRefcnt rather than frozen functions).  I should write patches to remove both.

I'm a little concerned about the dynamic_cast being in a macro that gets inlined just about everywhere in debug builds; I don't know how much (debug-only) codesize that yields.  There's a simple alternative, which would be, instead of changing the semantics of NS_LogCOMPtr{AddRef,Release}, add a new NS_LogSmartPtr{AddRef,Release}, and have the first pair just do the dynamic_cast and then forward the rest to the latter.

This would mean introducing two more debug-only entries to the XPCOM glue frozen functions table.  I'm curious what Benjamin thinks about that.

I'm also curious what the actual codesize increase from the dynamic_cast<void*> is.  I've never looked into how that gets compiled.

r=dbaron either way (i.e., as-is, or with the two new entries, although you'd probably want bsmedberg's ok on the two new entries)

Comment 51

[(no longer active)]

Attachment: Part 1: Make trace-refcount logging functions accept a void* argument; r=dbaron

Comment 52

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

Comment on attachment 8379423 [details] [diff] [review]
Part 2: Add RefCountType, a type compatible with nsrefcnt, to MFBT; r=dbaron

r=dbaron, although you probably want to run this by an mfbt owner

Comment 53

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

Comment on attachment 8379424 [details] [diff] [review]
Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

>+#if defined(MOZILLA_INTERNAL_API) && defined(DEBUG)

drop the "&& defined(DEBUG)" to reduce the risk of people dealing with debug vs non-debug header inclusion differences causing compilation errors in one or the other.  (I thought this was in our coding style guidelines, but seems to have been removed.)

>+// When building code that gets compiled into Gecko, try to use the
>+// trace-refcount leak logging facilities.
>+#if defined(MOZILLA_INTERNAL_API) && defined(DEBUG)

use NS_BUILD_REFCNT_LOGGING rather than DEBUG, throughout.

>+  private:
>+    // The default implementation of typeName, child classes are supposed to
>+    // override this.

; rather than ,


Otherwise looks good.

Comment 54

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

Comment on attachment 8379425 [details] [diff] [review]
Part 4: Add trace-refcount support to mozilla::RefPtr; r=dbaron

This isn't going to work, since it doesn't ensure that the pointer identity you pass here matches the pointer identity used in the previous part.

When I was commenting yesterday, I was thinking that RefPtr<T> was more tied to RefCounted<T> than it is (i.e., not at all).  Sorry, I was commenting too quickly.

If you want to use the pointer-identity-based logging, you'd need to do it in a template specialization that only applies to objects derived from RefCounted<T>; in those cases, you know what pointer identity is passed to the NS_LogAddRef/NS_LogRelease functions, and thus you can pass that same identity to NS_LogCOMPtrRelease/NS_LogCOMPtrAddRef.

It's also lower priority than the previous patch, so maybe not important to do right now.

Comment 55

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

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #50)
> So one thing here is that you're patching a bunch of code that's completely
> unused (nsTraceRefcnt::Log*, which we should have removed ages ago, and
> nsTraceRefcntImpl::Log*, which we should have removed when we unfroze XPCOM
> glue binary compatibility, which, ages ago, used nsITraceRefcnt rather than
> frozen functions).  I should write patches to remove both.

I put this in bug 975295.


Also, I'd note if you don't do patch 4 right now, you also don't need patch 1.

Comment 56

[[e-mail]]

Comment on attachment 8379424 [details] [diff] [review]
Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

Not sure why you made typeName() a member function, rather than static:

>+    const char* typeName() const { return "UnknownRefCounted<T>"; }
static const char* typeName() { return "UnknownRefCounted<T>"; }

>+                                        static_cast<const T*>(this)->typeName(),
T::typeName(),

>+                                         static_cast<const T*>(this)->typeName());
T::typeName());

>+  const char* typeName() const { return #T; }
static const char* typeName() { return #T; }

>+      return ptr->typeName();
return T::typeName();

(Shame VC10 doesn't allow static const char* typeName;)

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a29b5994e73a
https://hg.mozilla.org/mozilla-central/rev/60c0dbc3c8c5

Comment 58

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #49)
> Why do we need a nsDocShell::typeName for a WeakReference<nsDocShell>,
> exactly?

Because WeakReference is a RefCounted object, and will therefore require a typeName(), and the only way to get one would be to ask the template argument, nsDocShell in this case: <https://bugzilla.mozilla.org/attachment.cgi?id=8379424&action=diff#a/mfbt/WeakPtr.h_sec2>

Comment 59

[(no longer active)]

Comment on attachment 8379425 [details] [diff] [review]
Part 4: Add trace-refcount support to mozilla::RefPtr; r=dbaron

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #54)
> Comment on attachment 8379425 [details] [diff] [review]
> Part 4: Add trace-refcount support to mozilla::RefPtr; r=dbaron
> 
> This isn't going to work, since it doesn't ensure that the pointer identity
> you pass here matches the pointer identity used in the previous part.
> 
> When I was commenting yesterday, I was thinking that RefPtr<T> was more tied
> to RefCounted<T> than it is (i.e., not at all).  Sorry, I was commenting too
> quickly.
> 
> If you want to use the pointer-identity-based logging, you'd need to do it
> in a template specialization that only applies to objects derived from
> RefCounted<T>; in those cases, you know what pointer identity is passed to
> the NS_LogAddRef/NS_LogRelease functions, and thus you can pass that same
> identity to NS_LogCOMPtrRelease/NS_LogCOMPtrAddRef.
> 
> It's also lower priority than the previous patch, so maybe not important to
> do right now.

Hmm, you're right.  Sorry that I didn't pay attention to this earlier.  I think that at this point I'm inclined to shelve the part 1 and part 4 patch for now.

Comment 60

[(no longer active)]

(In reply to [e-mail] from comment #56)
> Comment on attachment 8379424 [details] [diff] [review]
> Part 3: Add trace-refcount logging for AddRef and Release in RefCounted
> objects; r=dbaron
> 
> Not sure why you made typeName() a member function, rather than static:

OK, I'll do that.

Comment 61

[(no longer active)]

Even more spraying: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc033be15f37

Comment 62

[(no longer active)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #53)
> >+// When building code that gets compiled into Gecko, try to use the
> >+// trace-refcount leak logging facilities.
> >+#if defined(MOZILLA_INTERNAL_API) && defined(DEBUG)
> 
> use NS_BUILD_REFCNT_LOGGING rather than DEBUG, throughout.

That requires nscore.h.  Instead, I'll do DEBUG||FORCE_BUILD_REFCNT_LOGGING.

Comment 63

[Nathan Froyd [:froydnj]]

Comment on attachment 8379423 [details] [diff] [review]
Part 2: Add RefCountType, a type compatible with nsrefcnt, to MFBT; r=dbaron

Review of attachment 8379423 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes below.

::: mfbt/RefCountType.h
@@ +7,5 @@
> +#ifndef mozilla_RefCountType_h
> +#define mozilla_RefCountType_h
> +
> +/**
> + * RefCountType is Mozilla's reference count type.

Please call this MozRefCountType since this has to be "namespaced" for C code.

@@ +17,5 @@
> + * as well, in order to be able to use the leak detection facilities
> + * that are implemented by XPCOM.
> + *
> + * The following ifdef exists to maintain binary compatibility with
> + * IUnknown.

Nit: can you note that this is IUnknown from Microsoft's COM stuff?

@@ +23,5 @@
> + * Note that this type is not in the mozilla namespace so that it is
> + * usable for both C and C++ code.
> + */
> +#ifdef XP_WIN
> +typedef unsigned long RefCountType;

Don't forget to rename here too! ;)

Comment 64

[Boris Zbarsky [:bzbarsky]]

> and the only way to get one would be to ask the template argument

Why?  Why can't it just return "WeakReference" or whatnot?

Comment 65

[(no longer active)]

Attachment: Part 0.1: Uninline nsIPresShell::SetForwardingContainer so that it won't need the full definition of nsDocShell; r=bzbarsky

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8379829 [details] [diff] [review]
Part 0.1: Uninline nsIPresShell::SetForwardingContainer so that it won't need the full definition of nsDocShell; r=bzbarsky

r=me

Comment 67

[(no longer active)]

(In reply to comment #64)
> > and the only way to get one would be to ask the template argument
> 
> Why?  Why can't it just return "WeakReference" or whatnot?

FWIW the reason that is not an option is that these names are used as the keys in the bloat entry table so they actually need to be unique per type.

Comment 68

[(no longer active)]

Comment on attachment 8379829 [details] [diff] [review]
Part 0.1: Uninline nsIPresShell::SetForwardingContainer so that it won't need the full definition of nsDocShell; r=bzbarsky

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5af9a67885

Comment 69

[(no longer active)]

Comment on attachment 8379423 [details] [diff] [review]
Part 2: Add RefCountType, a type compatible with nsrefcnt, to MFBT; r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/80b3309661a1

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc033be15f37
https://hg.mozilla.org/mozilla-central/rev/babff8d1437b

Comment 71

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b5af9a67885
https://hg.mozilla.org/mozilla-central/rev/80b3309661a1

Comment 72

[(no longer active)]

I realized that removing the assertions in part 1 was a mistake, and that the assertions should really work on a signed type.  Here's the fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/801aadc11572

Comment 73

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/801aadc11572

Comment 74

[(no longer active)]

One pattern that I discovered in moz2d code is something like:

class Base : public RefCounted<Base> {...};
class Derived : public Base {...};

With that, RefPtr<Base> objects are sometimes used to point to objects of type Derived, which confuses our type name code.  This means that the typeName() function should be virtual for those classes.  This patch adds MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME and sprays it across moz2d code:

https://hg.mozilla.org/integration/mozilla-inbound/rev/76407f0f10ba

Comment 75

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #72)
> I realized that removing the assertions in part 1 was a mistake, and that
> the assertions should really work on a signed type.  Here's the fix:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/801aadc11572

Oh, duh.  Sorry about that. :(

Comment 76

[(no longer active)]

I'm getting a whole bunch of weirdness which I'm trying to debug on the try server, and I am failing to do that because I cannot get proper assertion stacks on any platform.  Ted blamed bug 939610.

Comment 77

[(no longer active)]

David, can you please confirm if it's expected to not have HAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR on MSVC?

Comment 78

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #76)
> I'm getting a whole bunch of weirdness which I'm trying to debug on the try
> server, and I am failing to do that because I cannot get proper assertion
> stacks on any platform.  Ted blamed bug 939610.

Do you have https://hg.mozilla.org/mozilla-central/rev/72c0c955cf53 in your tree?

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #77)
> David, can you please confirm if it's expected to not have
> HAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR on MSVC?

It was certainly expected for older versions of MSVC.  But since we don't actually run our autoconf tests on MSVC but instead maintain the macros manually, I have no idea; you could try running the test from configure.in on current MSVC and see what happens.  (Annoyingly, it's an AC_TRY_RUN, so it also doesn't work when cross-compiling.)

Comment 79

[(no longer active)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #78)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #76)
> > I'm getting a whole bunch of weirdness which I'm trying to debug on the try
> > server, and I am failing to do that because I cannot get proper assertion
> > stacks on any platform.  Ted blamed bug 939610.
> 
> Do you have https://hg.mozilla.org/mozilla-central/rev/72c0c955cf53 in your
> tree?

Yes.

> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #77)
> > David, can you please confirm if it's expected to not have
> > HAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR on MSVC?
> 
> It was certainly expected for older versions of MSVC.  But since we don't
> actually run our autoconf tests on MSVC but instead maintain the macros
> manually, I have no idea; you could try running the test from configure.in
> on current MSVC and see what happens.  (Annoyingly, it's an AC_TRY_RUN, so
> it also doesn't work when cross-compiling.)

Hmm, do you know where that hardcoded value comes from?  When I run that test program, it returns 0, so perhaps we should update it?

Comment 80

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

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #79)
> > (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> > emailapocalypse) from comment #76)
> > > I'm getting a whole bunch of weirdness which I'm trying to debug on the try
> > > server, and I am failing to do that because I cannot get proper assertion
> > > stacks on any platform.  Ted blamed bug 939610.

See the further progress there.

> Hmm, do you know where that hardcoded value comes from?  When I run that
> test program, it returns 0, so perhaps we should update it?

Nowhere, since we never had to hardcode it.  Maybe we should just drop support for compilers that don't support dynamic_cast<void*> ?

(I feel like there was a discussion of this in another bug somewhere, but can't find it.)

Comment 81

[(no longer active)]

(In reply to comment #80)
> > Hmm, do you know where that hardcoded value comes from?  When I run that
> > test program, it returns 0, so perhaps we should update it?
> 
> Nowhere, since we never had to hardcode it.  Maybe we should just drop support
> for compilers that don't support dynamic_cast<void*> ?

Yeah I think we should do that...  I'll file a bug about that.

Comment 82

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/76407f0f10ba

Comment 83

[(no longer active)]

Attachment: Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

Comment 84

[(no longer active)]

Attachment: Part 0.7: Emit the correct type name from FilterNodeLightingSoftware; r=jrmuizel

Comment 85

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8381412 [details] [diff] [review]
Part 0.7: Emit the correct type name from FilterNodeLightingSoftware; r=jrmuizel

Review of attachment 8381412 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/FilterNodeSoftware.cpp
@@ +533,5 @@
>      case FilterType::UNPREMULTIPLY:
>        filter = new FilterNodeUnpremultiplySoftware();
>        break;
>      case FilterType::POINT_DIFFUSE:
> +      filter = new FilterNodeLightingSoftware<PointLightSoftware, DiffuseLightingSoftware>("FilterNodeLightingSoftware<PointLight, DiffuseLighting>");

Is dropping the 'Software' suffix from the template parameters intentional?

Comment 86

[(no longer active)]

(In reply to comment #85)
> Comment on attachment 8381412 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8381412
> Part 0.7: Emit the correct type name from FilterNodeLightingSoftware;
> r=jrmuizel
> 
> Review of attachment 8381412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/FilterNodeSoftware.cpp
> @@ +533,5 @@
> >      case FilterType::UNPREMULTIPLY:
> >        filter = new FilterNodeUnpremultiplySoftware();
> >        break;
> >      case FilterType::POINT_DIFFUSE:
> > +      filter = new FilterNodeLightingSoftware<PointLightSoftware, DiffuseLightingSoftware>("FilterNodeLightingSoftware<PointLight, DiffuseLighting>");
> 
> Is dropping the 'Software' suffix from the template parameters intentional?

Yeah, it's shorter and it's just a name.

Comment 87

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

Comment on attachment 8381392 [details] [diff] [review]
Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

At least use snprintf instead of sprintf?

r=dbaron with that

Comment 88

[(no longer active)]

(In reply to comment #87)
> Comment on attachment 8381392 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8381392
> Part 3: Add trace-refcount logging for AddRef and Release in RefCounted
> objects; r=dbaron
> 
> At least use snprintf instead of sprintf?

Haha, ok will do.  FWIW once I land bug 976363, this is ready to go.

Comment 89

[(no longer active)]

Attachment: Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

Comment 90

[(no longer active)]

Attachment: Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

Comment 91

[(no longer active)]

Comment on attachment 8381412 [details] [diff] [review]
Part 0.7: Emit the correct type name from FilterNodeLightingSoftware; r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd9e9cf9646

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9dd9e9cf9646

Comment 93

[(no longer active)]

Attachment: Part 3: Add trace-refcount logging for AddRef and Release in RefCounted objects; r=dbaron

Comment 94

[(no longer active)]

More spraying: https://hg.mozilla.org/integration/mozilla-inbound/rev/181dd1dec767

Comment 95

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/181dd1dec767

Comment 96

[(no longer active)]

Last part: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ece485bb722

Comment 97

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6ece485bb722

Comment 98

[Reuben Morais [:reuben]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #72)
> I realized that removing the assertions in part 1 was a mistake, and that
> the assertions should really work on a signed type.  Here's the fix:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/801aadc11572

Sorry for bringing this bug back from the dead for what is mostly curiosity, but I was wondering what made you realize removing those assertions was a mistake?

The equivalent assert in NS_IMPL_ADDREF traces back to 1999, added to "help find uninitialized refcnts": https://github.com/mozilla/gecko-dev/commit/f891e5da6865739c876c360fd6eb0e98a4c1674e

Does it still serve the same purpose in mozilla::RefPtr?

Comment 99

[(no longer active)]

(In reply to Reuben Morais [:reuben] from comment #98)
> (In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #72)
> > I realized that removing the assertions in part 1 was a mistake, and that
> > the assertions should really work on a signed type.  Here's the fix:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/801aadc11572
> 
> Sorry for bringing this bug back from the dead for what is mostly curiosity,
> but I was wondering what made you realize removing those assertions was a
> mistake?

They will let you discover when the refcount goes negative.