Statically prevent do_QueryInterface to a super class

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
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
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.
Oh, good catch.  We should be using IsBaseOf there, I would think.
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).
See Also: → 1493276
Depends on: 1493737
Summary: Statically prevent QueryInterface to a super class → Statically prevent do_QueryInterface to a super class
See Also: → 1463907
Depends on: 1493791
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.
Flags: needinfo?(nfroyd)
(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>())`.
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).
Those sound promising. Thanks for the suggestions.
Flags: needinfo?(nfroyd)
Depends on: 1494047
Depends on: 1494079
Depends on: 1494111
Depends on: 1494127
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.
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.
Depends on: 1495814
Depends on: 1495820
Depends on: 1495912
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.
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
Attachment #9013022 - Attachment is obsolete: true
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
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.
Thanks, I'll start when things are "hotting up" ;-)
Pushed by amccreight@mozilla.com:
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
See Also: 1463907
Duplicate of this bug: 1463907
https://hg.mozilla.org/mozilla-central/rev/a5ebcd6b0fbe
https://hg.mozilla.org/mozilla-central/rev/4bada21c38bf
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.