1493276 - Tighten assertion in CallQueryInterface

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Andrew McCreight [:mccr8]]

CallQueryInterface statically asserts that you don't QI an object with type A to type B when A is the same as B, but we should also ban it when A is a subclass of B (unless B is nsISupports), because a QI is slower than a static cast.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1493276 - Statically prevent CallQueryInterface to a base class

If a class A is derived from a class B, then an instance of A can be
converted to an instance of class B via a static cast, so QI is not
needed. QIs are slower than static casts.

TestCallTemplates seems to be testing that CallQueryInterface compiles
even if the first argument's class is only ambiguously castable to
nsISupports, so I changed the second argument to be a class unrelated
to the concrete class.

I also removed some useless null checks on the return value of new.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44e5267358eb
Statically prevent CallQueryInterface to a base class r=froydnj

Comment 3

[Andrew McCreight [:mccr8]]

Jorg, this may or may not cause build failures in Thunderbird. They'll be static asserts at calls to CallQueryInterface, and you have to remove the CallQueryInterface and turn them into regular assignments as seen in my patch.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 9011932 [details]
Bug 1493276 - Statically prevent CallQueryInterface to a base class

Nathan Froyd [:froydnj] has approved the revision.

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Jorg, this may or may not cause build failures in Thunderbird. They'll be
> static asserts at calls to CallQueryInterface, and you have to remove the
> CallQueryInterface and turn them into regular assignments as seen in my
> patch.
Thanks for the warning. We have a few CallQueryInterface call sites, is there a way to tell which ones need replacement? So if they are build failures, I can apply the patch now, compile and see, right?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, exactly.  If it compiles, it's fine.  If not, it needs fixing.

Comment 7

[Andrew McCreight [:mccr8]]

Yeah. And any fixes can land in advance of whenever my patch lands on comm-central.

Comment 8

[Jorg K (CEST = GMT+2)]

I got at least two compile errors, so I'll file a bug and fix them, thanks again.

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/44e5267358eb