1493226 - Statically prevent do_QueryInterface to a super class

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Andrew McCreight [:mccr8]]

Boris would like to statically prevent people using a QI where a static cast would work. A static cast is better because it is faster.

He pointed out this instance as an example: https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/xul/XULDocument.cpp#2500,2529-2534

This could be done by making nsQueryInterface templated with a type, so that the consumers of it can statically reject that. There are apparently comments about not wanting to do this to avoid code bloat, so we'd either have to make sure that stuff gets inlined away so that isn't an issue, or make it debug only.

Comment 1

[Andrew McCreight [:mccr8]]

nsISupports is allowed, but no other classes. Though maybe in theory we could detect situations where non-ambiguous static casting to nsISupports is possible, either directly or via ToSupports.

bz pointed me at this equivalent requirement for CallQueryInterface:

https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/xpcom/base/nsISupportsUtils.h#127-129

Comment 2

[Andrew McCreight [:mccr8]]

The CallQueryInterface static_assert doesn't appear to be sufficient, as this is just fine:
  static_assert(!mozilla::IsSame<nsIContent, Element>::value, "blah blah");
IsSame seems to only check that the types are not identical, so I'll have to come up with something else here. Maybe IsBaseOf would work.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, good catch.  We should be using IsBaseOf there, I would think.

Comment 4

[Andrew McCreight [:mccr8]]

From bug 983719: "We could *probably* detect when T was a subtype of DestinationType [...] but I will leave that for a followup."

I guess I can try changing that code first (which I'd probably put in a separate bug).

Comment 5

[Andrew McCreight [:mccr8]]

Right now, there is only a single instantiation of do_QueryInterface that is actually used:

inline nsQueryInterface
do_QueryInterface(nsISupports* aRawPtr)
{
  return nsQueryInterface(aRawPtr);
}

The problem I'm having is that once nsQueryInterface is templatized (to contain the type it was QI'd from) then the above becomes more like:

template<class T>
inline nsQueryInterface<T>
do_QueryInterface(T* aRawPtr)
{
  return nsQueryInterface<T>(aRawPtr);
}

The problem is that it seems like the current definition relied on an implicit conversion in the various smart pointer classes (nsCOMPtr, RefPtr, etc.) which doesn't seem to kick in, so I've had to define specific overloads for nsCOMPtr, etc., which isn't too bad, but there's quite a few of them (DebugOnly, NonNull, etc.)

Nathan, is there some nicer way to deal with this kind of issue that you are aware of? Thanks.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> The problem is that it seems like the current definition relied on an
> implicit conversion in the various smart pointer classes (nsCOMPtr, RefPtr,
> etc.) which doesn't seem to kick in, [...]

Maybe you can take a reference to the potentially-smart pointer, and then cast it to `nsISupports*` inside the function?
And if you need to extract the pointed-at type, this should work: `decltype(&*aRefToPtr)`, or `decltype(&*mozilla::DeclVal<T>())`.

Comment 7

[Boris Zbarsky [:bzbarsky]]

You could cheat a bitL

template<class U, class T>
inline nsQueyrInterface<T>
do_QueryInterface<const U<T>& aRawPtr>
{
  return nsQueryInterface<T>(aRawPtr.get());
}

(or static_cast instead of .get() may be better).

Comment 8

[Andrew McCreight [:mccr8]]

Those sound promising. Thanks for the suggestions.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Statically prevent trivial calls to do_QueryInterface. WIP

Here's my partially working fix. It builds in debug builds, but in opt
builds nsCOMPtr uses a different implementation, so I need to adjust
things to somehow strip out the type information.

Comment 10

[Andrew McCreight [:mccr8]]

My first pass at a version that actually works in opt builds causes an increase in text size of 46984, which doesn't sound too great. I'll see if I can figure out how to get rid of more of the template stuff in opt builds.

Comment 11

[Andrew McCreight [:mccr8]]

My new version of the checking should generate the same code in opt builds as we do now, and in fact shows the same results when the |size| command is run on libxul.so. I still have some QIs to remove before this can land, but I've gotten a clean try run on Linux, and also managed to build on OSX, so I think this is ready for review.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Bug 1493226, part 1 - Statically prevent trivial calls to do_QueryInterface

This patch adds a static assert to enforce that do_QueryInterface is
not used to go from a class to a base class, because that can be done
more efficiently via a static_cast. This is done by putting the type
of the source into the nsQueryInterface type. Once that is done, it is
easy to check the source and destination type. This has to be done
carefully so that in non-DEBUG builds, where NSCAP_FEATURE_USE_BASE is
defined, we don't generate any additional code.

The first step is to rename nsQueryInterface to
nsQueryInterfaceISupports. In DEBUG builds, I then add a new subtype
nsQueryInterface<T>, where T is the type of the object we are going to
QI. This class is a thin wrapper around nsQueryInterfaceISupports that
only forwards operations to the base class.

The main bit of trickery here is PointedToType<T>, which is needed to
get the type parameter for nsQueryInterface. This dereferences the
type, gets the address of it, then does RemovePointer. This is needed
because a wide variety of pointer types are passed in to
do_QueryInterface, including RefPtr<>, nsCOMPtr<>, raw pointers, and
OwningNonNull<>. PointedToType<RefPtr<T>> is equal to T,
PointedToType<T*> is equal to T, and so on.

In NSCAP_FEATURE_USE_BASE builds (opt), we only use
nsQueryInterfaceISupports, but in debug builds we use
nsQueryInterface<> where possible. The latter is also used for the
nsCOMPtr<nsISupports> overload, because we can always QI to
nsISupports, because that is sometimes used for canonicalization.

Another gross bit is that Assert_NoQueryNeeded can no longer use
nsCOMPtr<>, because it works by QIing T to T, which is banned by the
static analysis. Instead I had to reimplement it by hand.

Depends on D7527

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: Bug 1493226, part 2 - Statically prevent trivial calls to do_QueryInterface that returns an nsresult

This is the same as the other patch, except that it is applied to the
case where the QI returns an nsresult.

In addition, I marked the WithError helper class as being stack-only.

Depends on D7553

Comment 14

[Andrew McCreight [:mccr8]]

Jorg, this will probably also break Thunderbird when it lands. This is like bug 1493276, except for do_QueryInterface. Basically the fix is to replace things like |x = do_QueryInterface(y)| with |x = y| where you are getting the static asserts. Note that the dependent bugs have not all landed yet, so if you are working on this you'll want those in your tree, too.

Comment 15

[Jorg K (CEST = GMT+2)]

Thanks, I'll start when things are "hotting up" ;-)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5ebcd6b0fbe
part 1 - Statically prevent trivial calls to do_QueryInterface r=froydnj
https://hg.mozilla.org/integration/autoland/rev/4bada21c38bf
part 2 - Statically prevent trivial calls to do_QueryInterface that returns an nsresult r=froydnj

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a5ebcd6b0fbe
https://hg.mozilla.org/mozilla-central/rev/4bada21c38bf