181387 - do_QueryInterface(this) doesn't work with multiple inheritance

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Bradley Baetz (:bbaetz)]

bz mentioned this to me on IRC:

|do_QueryInterface(this)| fails when |this| has multiple inheritance, because it
can't be converted to an nsISupports* unambiguously.

CallQueryInterface was fixed in bug 87735 for the same problem.
(|CallQueryInterface(this, getter_AddRefs(something))| doesn't work, though,
because the second template param is a T**, and already_Addrefed<t> doesn't
match, because conversion operators arent' taken into account when deducing
template parameter types. So thats not a workarround. |CallQueryInterface(this,
NS_STATIC_CAST(something**, getter_AddRefs(something)))| does work, but its ugly.

As in bug 87735, we don't care that we can't convert |this| to an nsISupports*,
since we only need to be able to call this->QueryInterface unambiguously, which
we can. The fix is to change do_QueryInterface (and thus nsQueryInterface) to be
templated, and use T* rather than nsISupports* as the param.

There are several issues with this.

- We'll get one copy of the function per T* do_QI is called on. do_QI should be
inlinable, as should the nsQueryInterface constructor, but operator() is
currently defined in a .cpp, and that would have to be moved to the .h. Doing so
may increase binary size (although only for do_QI(rawptr), not
do_QI(nsCOMPtr<T>), so it won't affect most places), but OTOH it may give the
compiler the opportunity to inline this, since
|do_QueryInterface(non-null-ptr)|'s nsQueryInterface::operator() simply becomes
|return mRawPtr->QueryInterface()|, where mRawPtr may be inlinable. I don't know
if any compiler does do that, though - we'd have to test. (operator() is
virtual, which is another issue, but we know the real type since its just been
constructed so that shouldn't be an issue. I think later versions of gcc may be
able to do the inlining, buit there are lots of levels of indirection involved
which will complicate things) Our release compiler with -O almost certainly
won't do that. We'd definately need to test this on other compilers

- ISTR that nsCOMPtr.h was FROZEN. Theres no comment in the header file though,
so I may be wrong. I'm also not sure if that applied to just nsCOMPtr<T>, or to
the helpers as well. dougt?

- nsCOMPtr<T> has a debug-only |nsCOMPtr( const nsQueryInterface& helper )|
constructor. Changing nsQueryInterface would change the mangling, but its debug
only, so any thrid party 'stuff' built against that debug header wuldn't have
worked in a release build anyway. Or am I missing something? If nsCOMPtr<T> is
frozen, does that affect this?

Thoughts?

Comment 1

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

It seems like it should be rare to want to call QueryInterface on an object
whose concrete type is known, since you already know what interfaces are
supported.  (There are certainly uses, though, such as getting the canonical
nsISupports pointer.)  In other words, is this too minor a problem to be worth
that much trouble to fix, when one can just NS_STATIC_CAST around the problem?

Comment 2

[Bradley Baetz (:bbaetz)]

dbaron: possibly. If you want to WONTFIX this I won't mind - I just filed this
on bhalf of bz.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Just to clarify why I need this...  I have an nsFoo.  nsBar and nsBaz both
inherit from nsFoo and both implement nsIDOMNode.  nsFoo does not inherit from
nsIDOMNode and in fact does not even implement QueryInterface.   I am inside an
nsFoo member function.  I know that my object is not an actual nsFoo object,
since nsFoo has pure virtual functions on it.  But I don't know whether I am
nsBar or nsBaz, so it seems that the only sane way to get nsIDOMNode our of
"this" is to QI.... The reason I need an nsIDOMNode, of course, is to call a
function that takes nsIDOMNode (and I can't just pass "this" since again nsFoo
does not inherit from nsIDOMNode).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, that said I would be fine with a WONTFIX as long as we document the
workaround somewhere.  ;)

Comment 5

[Doug Turner (:dougt)]

boris, feel free to document this problem and attach a patch.

Comment 6

[Scott Collins]

This is essentially a special case of the situation I documented here:
<news://news.mozilla.org:119/[e-mail]>

Comment 7

[Scott Collins]

In case it's not immediately apparent, the reason this is a special case is
because |this| is pretty much always a concrete class from the compiler's
perspective.  Therefore, the rules for concrete classes apply, and the posting
noted above summarize those rules.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Right.  I'm just looking for a way to call this->QueryInterface without having
to use NS_GET_IID.... ;)  As comment 0 points out, the bug is really about
CallQI, not do_QI...

Comment 9

[[e-mail]]

Fixed by bug 538964?

nsCOMPtr<nsIDOMNode> thisAsDOMNode(do_QueryObject(this));

Comment 10

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

You shouldn't have to use a function called do_QueryObject to call QueryInterface.