1057224 - Enforce passing a valid class name to MOZ_COUNT_CTOR

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ralph Giles (:rillian)]

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.

Comment 1

[Ralph Giles (:rillian)]

Attachment: Use Assertion not RDF_Assertion

Once file in the rdf code doesn't currently use a class name.

Comment 2

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 3

[(no longer active)]

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.

Comment 4

[(no longer active)]

(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.

Comment 5

[Ralph Giles (:rillian)]

Attachment: static assert the type is a class

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.

Comment 6

[(no longer active)]

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.

Comment 7

[Ralph Giles (:rillian)]

Attachment: static assert the type is a class

Address comments, carrying forward r=ehsan. Thanks for the reviews!

Comment 8

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d06c2e71aca2
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f60f94d4d3c

Comment 9

[Ralph Giles (:rillian)]

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.

Comment 10

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=eac6cb7e8e9d

Comment 11

[Ralph Giles (:rillian)]

Attachment: Disable MOZ_ASSERT_CLASSNAME on gcc < 4.7

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

Comment 12

[(no longer active)]

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.

Comment 13

[Ralph Giles (:rillian)]

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.

Comment 14

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc83994de57
https://hg.mozilla.org/integration/mozilla-inbound/rev/de6c64ee1e21
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6bc20c68432

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/8dc83994de57
https://hg.mozilla.org/mozilla-central/rev/de6c64ee1e21
https://hg.mozilla.org/mozilla-central/rev/f6bc20c68432