Tighten assertion in CallQueryInterface

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

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.
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.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44e5267358eb
Statically prevent CallQueryInterface to a base class r=froydnj
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 on attachment 9011932 [details]
Bug 1493276 - Statically prevent CallQueryInterface to a base class

Nathan Froyd [:froydnj] has approved the revision.
Attachment #9011932 - Flags: review+
(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?
Yes, exactly.  If it compiles, it's fine.  If not, it needs fixing.
Yeah. And any fixes can land in advance of whenever my patch lands on comm-central.
I got at least two compile errors, so I'll file a bug and fix them, thanks again.
Depends on: 1494742
https://hg.mozilla.org/mozilla-central/rev/44e5267358eb
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.