Closed Bug 1493276 Opened 3 years ago Closed 3 years ago

Tighten assertion in CallQueryInterface

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file)

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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.