Closed Bug 1057224 Opened 5 years ago Closed 5 years ago

Enforce passing a valid class name to MOZ_COUNT_CTOR

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: rillian, Assigned: rillian)

Details

Attachments

(3 files, 2 obsolete files)

I recently had a bug where I typoed the classname in MOZ_COUNT_DTOR, resulting in spurious leak reports. It would be nice if the compiler could verify these are correct.
Once file in the rdf code doesn't currently use a class name.
Assignee: nobody → giles
This works for MOZ_COUNT_CTOR(int), but for typos like I'm trying to catch it never gets to the static assert:

error: use of undeclared identifier 'FooBarNoClass'
  MOZ_COUNT_CTOR(FooBarNoClass);
                 ^
../../../dist/include/nsISupportsImpl.h:162:34: note: expanded from macro 'MOZ_COUNT_CTOR'
  static_assert(mozilla::IsClass<_type>::value,

Ehsan, any better ideas here? It would also be nice to verify the class name matches the type of *this. I'll see if I can get anything useful out of typeinfo<>.name() and __builtin_strcmp.
Attachment #8477231 - Flags: feedback?(ehsan)
Comment on attachment 8477231 [details] [diff] [review]
static assert the type is a class

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

::: xpcom/glue/nsISupportsImpl.h
@@ +159,5 @@
>  // Refcount logging is preferred.
>  #define MOZ_COUNT_CTOR(_type)                                 \
>  do {                                                          \
> +  static_assert(mozilla::IsClass<_type>::value,               \
> +      "Name " #_type " passed to MOZ_COUNT_CTOR is not a class"); \

This looks good, but you need to do the same for MOZ_COUNT_DTOR for sanity's sake as well.
Attachment #8477231 - Flags: feedback?(ehsan) → feedback+
(In reply to Ralph Giles (:rillian) from comment #2)
> Created attachment 8477231 [details] [diff] [review]
> static assert the type is a class
> 
> This works for MOZ_COUNT_CTOR(int), but for typos like I'm trying to catch
> it never gets to the static assert:
> 
> error: use of undeclared identifier 'FooBarNoClass'
>   MOZ_COUNT_CTOR(FooBarNoClass);
>                  ^
> ../../../dist/include/nsISupportsImpl.h:162:34: note: expanded from macro
> 'MOZ_COUNT_CTOR'
>   static_assert(mozilla::IsClass<_type>::value,
> 
> Ehsan, any better ideas here? It would also be nice to verify the class name
> matches the type of *this. I'll see if I can get anything useful out of
> typeinfo<>.name() and __builtin_strcmp.

Remember that typeinfo requires RTTI and we build our code with RTTI disabled.  Also, that would actually be the wrong thing to check, because RTTI gives you the dynamic type of the object at runtime, but you pass the static type to MOZ_COUNT_CTOR (think of a base class which has MOZ_COUNT_CTOR in its constructor and you derive a class from it and create an instance of the derived class.  There, the type of *this would be the derived class, but you have passed the name of the base class to MOZ_COUNT_CTOR.
Yes, of course it should be applied to the other macros.

I still suspect there's template magic we could do to improve the error message, but this is good enough for now.
Attachment #8477231 - Attachment is obsolete: true
Attachment #8477582 - Flags: review?(ehsan)
Attachment #8477228 - Flags: review?(nfroyd)
Comment on attachment 8477582 [details] [diff] [review]
static assert the type is a class

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

::: xpcom/glue/nsISupportsImpl.h
@@ +153,5 @@
>    NS_LogRelease((_p), (_rc), (_type))
>  
> +#include "mozilla/TypeTraits.h"
> +#define MOZ_ASSERT_CLASSNAME(_type)             \
> +do {                                            \

This macro shouldn't need a do while.

@@ +155,5 @@
> +#include "mozilla/TypeTraits.h"
> +#define MOZ_ASSERT_CLASSNAME(_type)             \
> +do {                                            \
> +  static_assert(mozilla::IsClass<_type>::value, \
> +      "Token '" #_type "' is not a class type.");      \

Nit: please make the starting quote line up with "mozilla" in the line above, and also fix the backslashes so that they all line up vertically.
Attachment #8477582 - Flags: review?(ehsan) → review+
Attachment #8477228 - Flags: review?(nfroyd) → review+
Address comments, carrying forward r=ehsan. Thanks for the reviews!
Attachment #8477582 - Attachment is obsolete: true
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/79356188391a

There is of course code on other platforms which needs updating as well.
Work around failure with local class names in nsGonkCameraController on ICS by disabling the check for gcc versions prior to 4.7.

Green on try https://tbpl.mozilla.org/?tree=Try&rev=305a35a817cd
Attachment #8477918 - Flags: review?(ehsan)
Comment on attachment 8477918 [details] [diff] [review]
Disable MOZ_ASSERT_CLASSNAME on gcc < 4.7

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

Sigh.  I don't think this is worth adding a configure check for, so this is fine.
Attachment #8477918 - Flags: review?(ehsan) → review+
Thanks. I also considered moving the offending class out of local scope, but rejected that for the same reasons: the issue is with one older toolchain, and it doesn't make sense to make the code uglier just to protect that.
You need to log in before you can comment on or make changes to this bug.