Closed Bug 983719 Opened 6 years ago Closed 6 years ago

make CallQueryInterface detect when the QI would be trivial

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: froydnj, Unassigned)

Details

Attachments

(1 file)

We actually do this some non-trivial number of times.  Let's address it at the
source.
C++ templates to the rescue.  We could *probably* detect when T was a subtype
of DestinationType, for catching things like:

  nsRefPtr<T> thing = new T(...)
  /* QI thing to DestinationType** outparam */

but I will leave that for a followup.
Attachment #8391306 - Flags: review?(benjamin)
Comment on attachment 8391306 [details] [diff] [review]
make CallQueryInterface detect when the QI would be trivial

This is incorrect at least for nsISupports where the point of CallQueryInterface may be to get the canonical nsISupports* and not a static-cast of the current interface.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> This is incorrect at least for nsISupports where the point of
> CallQueryInterface may be to get the canonical nsISupports* and not a
> static-cast of the current interface.

But in such a case, we wouldn't be matching the <T,T> specialization, would we?  In a get-canonical-ISupports case, we'd have some non-nsISupports pointer and we'd take the same path we always did.  (My understanding of this case may be incomplete.)

Also, I have only lightly tested this patch locally; it's possible try might turn up some real problems...
bent suggested on IRC that we should instead do:

CallQueryInterface(T*, DestinationType**)
{
  static_assert(!mozilla::IsSame<T, DestinationType>::value, "don't QI to the same class!");

  ...as before
}

and fix all the bogus places.
(In reply to Nathan Froyd (:froydnj) from comment #3)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> > This is incorrect at least for nsISupports where the point of
> > CallQueryInterface may be to get the canonical nsISupports* and not a
> > static-cast of the current interface.
> 
> But in such a case, we wouldn't be matching the <T,T> specialization, would
> we?  In a get-canonical-ISupports case, we'd have some non-nsISupports
> pointer and we'd take the same path we always did.

OK, so after some reading, I guess we can have the situation where:

  nsISupports* ptr = ...; // from somewhere
  nsISupports* canonical;
  CallQueryInterface(ptr, &canonical);
  // ptr may be != to canonical

So we could just disable the <T,T> specialization for T==nsISupports.
Benjamin, what do you want to do here:

- Add static_asserts to CallQueryInterface to prevent trivial casts and fixup all callers; or
- Make CallQueryInterface smart enough to optimize trivial instances away, but treating nsISupports carefully.

I think I lean slightly towards number 1.
Flags: needinfo?(benjamin)
#1, I think. We should continue to allow CallQueryInterface to nsISupports* because it's not trivial by definition.
Flags: needinfo?(benjamin)
OK, filing bug 984466 for that work, then.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE
Attachment #8391306 - Flags: review?(benjamin)
You need to log in before you can comment on or make changes to this bug.