87735 - CallQueryInterface doesn't support classes implementing mutiple interfaces

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

CallQueryInterface has a declaration of:

1255 template <class DestinationType>
1256 inline
1257 nsresult
1258 CallQueryInterface( nsISupports* aSource, DestinationType** aDestination )

This requires that a class be unambiguously convertable to nsISupports, which
isn't the case if it implements multiple interfaces. The only requirement should
be that ->QueryInterface is unambiguous.

This is normally not a problem, because interfaces don't support MI, and mozilla
mainly uses interface pointers. However, if you want to use this for CloneNode,
or NS_NewFoo, you end up with a pointer to the concrete class. You can
NS_STATIC_CAST the pointer to the appropriate parent class, but that shouldn't
be needed.

The signature should be changed to something like (untested):

template <typename T, class DestinationType>
inline nsresult
CallQueryInterface(T* aSource, DestinationType** aDestination)

Comment 1

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

Attachment: patch (needs more testing)

Comment 2

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

I have a patch that also fixes CallQueryReferent.  I'm going to try to test in
on gcc 2.7.2.3 sometime...

Comment 3

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

Attachment: patch with changes to CallQueryReferent as well

Comment 4

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

gcc 2.7.2.3 seems to be happy -- my build finish and I tried one use of
CallGetService on it (not nearly as many permutations as I tried on Windows).

I'm not sure whether I need to do more testing here.  I should probably check
that it builds on Mac -- I guess I'll do that now...

Comment 5

[jag (Peter Annema)]

+// a type-safe shortcut for calling the |QueryInterface()| member function
+// T must inherit from nsISupportsWeakReference, but the cast may be
+// ambiguous.
+template <class T, class DestinationType>
inline
 nsresult
-CallQueryReferent( nsIWeakReference* aSource, T** aDestination )
+CallQueryReferent( T* aSource, DestinationType** aDestination )
   {
     NS_PRECONDITION(aSource, "null parameter");
     NS_PRECONDITION(aDestination, "null parameter");
 
-    return aSource->QueryReferent(NS_GET_IID(T), (void**)aDestination);
+    return aSource->QueryReferent(NS_GET_IID(DestinationType),
+                                  NS_REINTERPRET_CAST(void**, aDestination));
   }




In the comment I think you meant QueryReferent.



+template <class DestinationType>
+inline
+nsresult
+CallGetService( const char *aContractID,
+                DestinationType** aDestination,
+                nsIShutdownListener* shutdownListener = nsnull)
+  {
+    NS_PRECONDITION(aContractID, "null parameter");
+    NS_PRECONDITION(aDestination, "null parameter");
+
+    return nsServiceManager::GetService( aContractID, nsnull,
+               NS_GET_IID(DestinationType),
+               NS_REINTERPRET_CAST(nsISupports**, aDestination),
+               shutdownListener);
+  }


I think that nsnull shouldn't be there.


+// NOTE: NS_WITH_SERVICE is deprecated.
+//  {
+//      nsCOMPtr<nsIMyService> service( do_GetService(kMyServiceCID, &rv) );
+//      if (NS_FAILED(rv)) return rv;


I think we would want to give the ContractID equivalents of these as examples.

Comment 6

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

> I think that nsnull shouldn't be there.

So what should I do instead?  I'd prefer not to make everybody type it
explicitly, especially since it's the last parameter.  Or should I do what I did
for CallCreateInstance and have the shutdown listener be an optional middle
parameter (which is different from, but probably better than, the parameter
order for nsComponentManager::CreateInstance)?

> I think we would want to give the ContractID equivalents of these as examples.

Do we prefer ContractIDs to CIDs now?  If so, is
s/kMyServiceCID/NS_MYSERVICE_CONTRACTID/g sufficient?

Comment 7

[jag (Peter Annema)]

Discussed which nsnull I meant with dbaron.

And yeah, I think that s/// is fine.

Comment 8

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

Attachment: corrected patch, plus test program

Comment 9

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

Attachment: oops, corrected another typo in a comment

Comment 10

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

The patch above, including test program, builds on VC6, gcc 3.0, and gcc
2.7.2.3.  (Well, my gcc 2.7.2.3 build hasn't finished yet, but the test program
builds fine.)  I'm happy with Mac from my testing yesterday -- I tested at least
2 or 3 of the new templates as well as doing a full build and making sure it
works, so since Mac is a good compiler I'm not at all worried.

Looking for review & super-review...

Comment 11

[jag (Peter Annema)]

+    return nsComponentManager::CreateInstance( aClass, aDelegate,
+               NS_GET_IID(DestinationType),
+               NS_REINTERPRET_CAST(void**, aDestination));

So call me nit-picky, but either remove the space after the first opening
parenthesis, or add one before the last closing parenthesis. :-)

For TestCallTemplates.cpp, could you use the MPL instead of the NPL?

r=jag

Comment 12

[Scott Collins]

what jag said :-)  sr=scc

Comment 13

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

Checked in 2001-07-04 11:55 PDT.