1010756 - Better compile errors if you mix up nsCOMPtr and nsRefPtr

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

If you use nsCOMPtr for a class like nsRange or Selection or Text, or use nsRefPtr for a class like nsINode, you will get some sort of exciting compiler error that is unlikely to be informative.  I recently ran into one when doing InsertElementAt in an nsTArray<nsCOMPtr<Text>> that took me a while to figure out, and I'm sure it would be hopeless for a novice Gecko hacker.

Could we improve this?  Is there a static_assert we could stick in nsCOMPtr/nsRefPtr constructors that would catch at least some of these cases and fail with a helpful error message?  I have no idea why these are failures to begin with, so I wouldn't have any idea what to assert on.

Comment 1

[(no longer active)]

What's the error that you're getting?  I'm sure we can improve things but I don't know enough about the current problem.  CCing Neil.

Comment 2

[[e-mail]]

(In reply to Ehsan Akhgari from comment #1)
> What's the error that you're getting?  I'm sure we can improve things but I
> don't know enough about the current problem.  CCing Neil.

GCC 4.8.2 spits out the following error:

> In instantiation of 'nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsBogusType]'
> error: incomplete type 'nsIParentInterface::COMTypeInfo<nsBogusType, void>' used in nested name specifier

This means that your nsCOMPtr<nsBogusType> should either be an nsCOMPtr<nsIParentInterface> or an nsRefPtr<nsBogusType>. Or, possibly, you need to complete the type by manually declaring an IID.

(In reply to Aryeh Gregor)
> I have no idea why these are
> failures to begin with, so I wouldn't have any idea what to assert on.

nsRefPtr<nsINode> isn't a compile error, although it should be a review error ;-)

nsCOMPtr is designed to template over interfaces, that is, classes with a declared IID. In particular interfaces allow you to call QueryInterface to obtain a pointer to a different interface. (Debug builds try to double-check that the pointers actually point to the interface you think they do.) Prior to bug 514280 it used to be all too easy for a concrete class to inherit an IID from an interface however were you to pass in such an IID you would actually get an nsIParentInterface* pointer back rather than the nsBogusType* pointer your code was assuming.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> although it should be a review error

No; WebIDL bindings use nsRefPtr for all refcounted types.

Comment 4

[[e-mail]]

(In reply to Boris Zbarsky from comment #3)
> > although it should be a review error
> 
> No; WebIDL bindings use nsRefPtr for all refcounted types.

Actually I know there are other exceptions to the rule, but I thought that my use of should was sufficiently weak to permit this.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Do you know of any way to write a static_assert that will catch at least some cases where nsCOMPtr<T> will fail to compile?  Probably using type_traits?

Comment 6

[[e-mail]]

(In reply to Aryeh Gregor from comment #5)
> Do you know of any way to write a static_assert that will catch at least
> some cases where nsCOMPtr<T> will fail to compile?  Probably using
> type_traits?

Well, it's fairly easy to make it so that nsCOMPtr<T> will fail to compile in release builds with the same error that it already fails to compile in debug builds i.e. error: incomplete type 'I::COMTypeInfo<T, void>' used in nested name specifier.

I've come up with a couple of approaches to make nsCOMPtr<T> fail to compile with the error: static assertion failed: nsCOMPtr may only be templated over interfaces

template <class T>
char (&TestForIID(T*))[sizeof(NS_GET_IID(T)) + 1];
char TestForIID(...);
template <class T>
class nsCOMPtr
  {
    static_assert(1 < sizeof(TestForIID(static_cast<T*>(nullptr)),
                  "nsCOMPtr may only be templated over interfaces");

template <class T, class U = T>
struct IsInterface {
  static const bool value = false;
};
template <class T>
struct IsInterface<T, typename T::template COMTypeInfo<T, void> > {
  static const bool value = true;
};
template <class T>
class nsCOMPtr
  {
    static_assert(IsInterface<T>::value,
                  "nsCOMPtr may only be templated over interfaces");

Note that this second version strictly only checks for the COMTypeInfo inner class and not for the IID itself.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to [e-mail] from comment #6)
> template <class T, class U = T>
> struct IsInterface {
>   static const bool value = false;
> };
> template <class T>
> struct IsInterface<T, typename T::template COMTypeInfo<T, void> > {
>   static const bool value = true;
> };
> template <class T>
> class nsCOMPtr
>   {
>     static_assert(IsInterface<T>::value,
>                   "nsCOMPtr may only be templated over interfaces");

I tried this, but got an error that U was being redeclared.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to [e-mail] from comment #6)
> template <class T>
> char (&TestForIID(T*))[sizeof(NS_GET_IID(T)) + 1];
> char TestForIID(...);
> template <class T>
> class nsCOMPtr
>   {
>     static_assert(1 < sizeof(TestForIID(static_cast<T*>(nullptr)),
>                   "nsCOMPtr may only be templated over interfaces");

And this one fails for all types, including those with IIDs.

Comment 9

[[e-mail]]

(In reply to Aryeh Gregor from comment #8)
> (In reply to comment #6)
> > template <class T>
> > char (&TestForIID(T*))[sizeof(NS_GET_IID(T)) + 1];
> > char TestForIID(...);
> > template <class T>
> > class nsCOMPtr
> >   {
> >     static_assert(1 < sizeof(TestForIID(static_cast<T*>(nullptr)),
> >                   "nsCOMPtr may only be templated over interfaces");
> 
> And this one fails for all types, including those with IIDs.

I probably originally compiled with gcc, which may have been more lenient, but MSVC tripped over it in two places. I've tweaked it slightly and MSVC likes this version:

template <class T>
char (&TestForIID(T*))[sizeof(NS_GET_TEMPLATE_IID(T)) + 1];
char TestForIID(...);
template <class T>
class nsCOMPtr final
  {
  // MSVC seems to need this intermediate value for some reason.
  static const bool value = 1 < sizeof(TestForIID(static_cast<T*>(nullptr)));
  static_assert(value, "nsCOMPtr may only be templated over interfaces");

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Did this compile successfully for you?  On Clang 3.5, all classes seem to fail the static_assert (including, e.g., nsIContent).

Comment 11

[[e-mail]]

Oh, I see what the problem is - the file I was testing on didn't forward-declare any of the interfaces it used. Let me investigate...

Comment 12

[[e-mail]]

Third time lucky? (xpcom/glue now compiles with this version; if I remember, I'll check back to see whether my build fails.)

template <class T>
char (&TestForIID(T*))[sizeof(NS_GET_TEMPLATE_IID(T)) + 1];
template <class T>
char TestForIID(...);
template <class T>
class nsCOMPtr final
  {
...
  ~nsCOMPtr()
  {
    static_assert(1 < sizeof(TestForIID<T>(nullptr)), "nsCOMPtr may only be templated over interfaces");
#ifndef NSCAP_FEATURE_USE_BASE
    NSCAP_LOG_RELEASE(this, mRawPtr);
    if (mRawPtr) {
      NSCAP_RELEASE(this, mRawPtr);
    }
#endif
  }

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Now it compiles, but doesn't serve the intended purpose, because if you try to do nsCOMPtr<nsRange> or whatever, it prints an error about ambiguous conversion to nsISupports* and never prints the static_assert.  Presumably this is because it's in the destructor instead of the class proper.

Comment 14

[[e-mail]]

Whoops, I'd managed to get parts of two different approaches there. Retrying my build with this (currently expecting it to fail in SmsChild.cpp which has an unused nsCOMPtr of a non-interface type):

template <class T>
char (&TestForIID(T*))[sizeof(NS_GET_TEMPLATE_IID(T)) + 1];
char TestForIID(...);
template <class T>
class nsCOMPtr final
  {
...
  ~nsCOMPtr()
  {
    static_assert(1 < sizeof(TestForIID(this)), "nsCOMPtr may only be templated over interfaces");
#ifndef NSCAP_FEATURE_USE_BASE
    NSCAP_LOG_RELEASE(this, mRawPtr);
    if (mRawPtr) {
      NSCAP_RELEASE(this, mRawPtr);
    }
#endif
  }

Comment 15

[[e-mail]]

(In reply to Aryeh Gregor from comment #13)
> Now it compiles, but doesn't serve the intended purpose, because if you try
> to do nsCOMPtr<nsRange> or whatever, it prints an error about ambiguous
> conversion to nsISupports* and never prints the static_assert.  Presumably
> this is because it's in the destructor instead of the class proper.

Maybe that's because you were using a debug build? Putting it in all the constructors is a bit awkward but would work. Putting it in the class doesn't work because the template type might still be incomplete.

Comment 16

[[e-mail]]

No, that version doesn't work either. Sigh...

Comment 17

[[e-mail]]

I have my fingers crossed for this version:

// The .m0 works around a bug in MSVC.
template<class T>
char (&TestForIID(T*))[sizeof(NS_GET_TEMPLATE_IID(T).m0)];
static char TestForIID(...);
template <class T>
class nsCOMPtr final
  {
...
  ~nsCOMPtr()
  {
    static_assert(1 < sizeof(TestForIID(get())), "nsCOMPtr may only be templated over interfaces");
#ifndef NSCAP_FEATURE_USE_BASE
    NSCAP_LOG_RELEASE(this, mRawPtr);
    if (mRawPtr) {
      NSCAP_RELEASE(this, mRawPtr);
    }
#endif
  }

Comment 18

[[e-mail]]

That didn't work either. What wasn't helping is that there are three cases:
1. The class does not inherit from an interface
2. The class inherits from an interface
3. The class is an interface
Then I found out I could make the code much simpler using decltype, so here it is:

template<class T>
char (&TestForIID(decltype(NS_GET_TEMPLATE_IID(T))*))[2];
template<class T>
char TestForIID(...);
template <class T>
class nsCOMPtr final
  {
...
  ~nsCOMPtr()
  {
    static_assert(1 < sizeof(TestForIID<T>(nullptr)), "nsCOMPtr may only be templated over interfaces");
#ifndef NSCAP_FEATURE_USE_BASE
    NSCAP_LOG_RELEASE(this, mRawPtr);
    if (mRawPtr) {
      NSCAP_RELEASE(this, mRawPtr);
    }
#endif
  }

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This still fails on every time for me (nsITimer, nsIObserver, nsIWidget, etc.).  :(  Clang 3.5 on Ubuntu 14.04 64-bit.

Comment 20

[[e-mail]]

Bah, MSVC 2013 bug - the extra parentheses introduced by NS_GET_IID turn the variable into an lvalue expression. Fortunately the fix is simple:

template<class T>
char (&TestForIID(decltype(&NS_GET_TEMPLATE_IID(T))))[2];
template<class T>
char TestForIID(...);
template <class T>
class nsCOMPtr final
  {
...
  ~nsCOMPtr()
  {
    static_assert(1 < sizeof(TestForIID<T>(nullptr)), "nsCOMPtr may only be templated over interfaces");
#ifndef NSCAP_FEATURE_USE_BASE
    NSCAP_LOG_RELEASE(this, mRawPtr);
    if (mRawPtr) {
      NSCAP_RELEASE(this, mRawPtr);
    }
#endif
  }

Comment 21

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Builds on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87b26b75f0d5

Comment 23

[[e-mail]]

Comment on attachment 8646476 [details] [diff] [review]
Patch

>-  nsCOMPtr<SmsMessage> message;
>+  nsRefPtr<SmsMessage> message;
Actually this variable is completely unused.

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 8646476 [details] [diff] [review]
Patch

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

r=me; if we can just have a definition of assert_validity, that'd be better than sprinkling assert_validity calls all over the place.

::: xpcom/glue/nsCOMPtr.h
@@ +426,5 @@
>  
>    nsCOMPtr()
>      : NSCAP_CTOR_BASE(0)
>    {
> +    assert_validity();

Do we need to "call" assert_validity all over the place, or can we just rely on assert_validity being instantiated (since it's declared inline in the class body) and triggering the static_assert thereby?

Comment 25

[[e-mail]]

(In reply to Nathan Froyd from comment #24)
> Do we need to "call" assert_validity all over the place, or can we just rely
> on assert_validity being instantiated (since it's declared inline in the
> class body) and triggering the static_assert thereby?

No, it doesn't get instantiated until you actually use it. Also, you can't put the assertion in the body of the nsCOMPtr template because the definition of T may not be complete yet. Although putting the assertion in the destructor avoids multiple copies, it delays the generation of the assertion until after any usage errors.

Comment 26

[Pulsebot]

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

Comment 27

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to [e-mail] from comment #25)
> No, it doesn't get instantiated until you actually use it.

Yes, that's what I found.

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/e5258869f5dd