Closed Bug 935778 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mattwoodrow, Assigned: ehsan.akhgari)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(5 files, 13 obsolete files)

12.41 KB, text/plain
Details
6.06 KB, patch
dbaron
: review+
froydnj
: review+
ehsan.akhgari
: checkin+
Details | Diff | Splinter Review
2.30 KB, patch
bzbarsky
: review+
ehsan.akhgari
: checkin+
Details | Diff | Splinter Review
5.11 KB, patch
jrmuizel
: review+
ehsan.akhgari
: checkin+
Details | Diff | Splinter Review
5.86 KB, patch
Details | Diff | Splinter Review
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.
Whiteboard: [MemShrink]
Why can't you just use nsRefPtr?
(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.
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.
(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).
(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?
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.
Surely moving code from XPCOM into MFBT and then including it back into XPCOM would be better?
Probably, yes. But I wasn't offering to do that :)
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [MemShrink] → [MemShrink:P1]
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?
> > 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.
(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...
We also have a similar problem in bug 956338 for turning on thread safety assertions for WeakPtr.
See Also: → 956338
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.
Flags: needinfo?(jwalden+bmo)
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.
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")
(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?
Yes, that should work.
I'll give that a shot.
Assignee: nobody → ehsan
Attachment #8378027 - Flags: review?(nfroyd)
Attachment #8378028 - Flags: review?(nfroyd)
Attachment #8378030 - Flags: review?(nfroyd)
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 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.
Attachment #8378028 - Flags: review?(nfroyd) → review-
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.
Attachment #8378030 - Flags: review?(nfroyd) → review-
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).
Attachment #8378027 - Flags: review?(nfroyd) → review-
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.)
Attachment #8378027 - Flags: review- → review+
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.
Attachment #8378027 - Flags: review+ → review-
(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.
(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?
Flags: needinfo?(dbaron)
(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.)
Flags: needinfo?(dbaron)
(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.
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.
Flags: needinfo?(bjacob)
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.
Keywords: leave-open
Attachment #8378027 - Attachment is obsolete: true
Attachment #8378028 - Attachment is obsolete: true
Attachment #8378030 - Attachment is obsolete: true
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.)
Attachment #8379406 - Flags: review?(bzbarsky)
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... :/
Attachment #8379406 - Attachment is obsolete: true
Attachment #8379406 - Flags: review?(bzbarsky)
Attachment #8379414 - Attachment is obsolete: true
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...
Attachment #8379416 - Flags: review?(bzbarsky)
Spray some MOZ_DECLARE_REFCOUNTED_TYPENAME across the tree:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a29b5994e73a
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.
Attachment #8379422 - Flags: review?(dbaron)
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.
Attachment #8379423 - Flags: review?(dbaron)
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.
Attachment #8379424 - Flags: feedback?(dbaron)
Attachment #8379425 - Flags: feedback?(dbaron)
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.
Why do we need a nsDocShell::typeName for a WeakReference<nsDocShell>, exactly?
Flags: needinfo?(ehsan)
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)
Attachment #8379422 - Flags: review?(dbaron) → review+
Flags: needinfo?(benjamin)
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
Attachment #8379423 - Flags: review?(dbaron) → review+
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.
Attachment #8379424 - Flags: feedback?(dbaron) → feedback+
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.
Attachment #8379425 - Flags: feedback?(dbaron) → feedback-
(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 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;)
(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>
Flags: needinfo?(ehsan)
Attachment #8379423 - Flags: review?(nfroyd)
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.
Attachment #8379425 - Attachment is obsolete: true
Attachment #8379422 - Attachment is obsolete: true
Flags: needinfo?(benjamin)
Attachment #8379486 - Attachment is obsolete: true
(In reply to neil@parkwaycc.co.uk 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.
(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 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! ;)
Attachment #8379423 - Flags: review?(nfroyd) → review+
> and the only way to get one would be to ask the template argument

Why?  Why can't it just return "WeakReference" or whatnot?
Attachment #8379416 - Attachment is obsolete: true
Attachment #8379416 - Flags: review?(bzbarsky)
Attachment #8379829 - Flags: review?(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
Attachment #8379829 - Flags: review?(bzbarsky) → review+
(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 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
Attachment #8379829 - Flags: checkin+
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
Attachment #8379423 - Flags: checkin+
Flags: needinfo?(bjacob)
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
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
(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. :(
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.
Depends on: 939610
David, can you please confirm if it's expected to not have HAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR on MSVC?
Flags: needinfo?(dbaron)
(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.)
Flags: needinfo?(dbaron)
(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?
Flags: needinfo?(dbaron)
(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.)
Flags: needinfo?(dbaron)
(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.
Depends on: 976363
Depends on: 976372
Depends on: 976392
Attachment #8379424 - Attachment is obsolete: true
Attachment #8381392 - Flags: review?(dbaron)
Attachment #8381412 - Flags: review?(jmuizelaar)
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?
Attachment #8381412 - Flags: review?(jmuizelaar) → review+
(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 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
Attachment #8381392 - Flags: review?(dbaron) → review+
(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.
Attachment #8381392 - Attachment is obsolete: true
Attachment #8381786 - Attachment is obsolete: true
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
Attachment #8381412 - Flags: checkin+
Blocks: 978472
Attachment #8381818 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6ece485bb722
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 985878
Flags: needinfo?(jwalden+bmo)
(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?
(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.
You need to log in before you can comment on or make changes to this bug.