538964 - Introduce do_QueryObject as helper to QueryInterface to concrete class types (for use with nsRefPtr)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[alexander :surkov (:asurkov)]

Applicable area is accessible module. Spun off the bug 538633 comment #5. Neil's suggestion:

+  nsRefPtr<nsAccessible> rootAcc =
>+    nsAccUtils::QueryObject<nsAccessible>(aRootAccessible);
I've possibly asked in a previous review and since forgotten, but why not use
nsAccUtils::QueryAccessible(aRootAccessible); ? In particular repeating the
type name is ugly. I think you can work around it by using a class and some
helper methods. At least, it keeps all the ugliness in one place:
template<class T>
class nsQueryObject
{
public:
  nsQueryObject(T* aRawPtr) : mRawPtr(aRawPtr) {}
  template<class U>
  operator already_AddRefed<U>() {
    U* object = nsnull;
    CallQueryInterface(mRawPtr, &object);
    return object;
  }
private:
  T* mRawPtr;
}
template<class T>
nsQueryObject<T> do_QueryObject(T* aRawPtr)
{
  return nsQueryObject<T>(aRawPtr);
}
template<class T>
nsQueryObject<T> do_QueryObject(nsCOMPtr<T> aCOMPtr)
{
  return nsQueryObject<T>(aCOMPtr);
}
template<class T>
nsQueryObject<T> do_QueryObject(nsRefPtr<T> aRefPtr)
{
  return nsQueryObject<T>(aRefPtr);
}
Then you can write things like
nsCOMPtr<nsIAccessibleHyperText> hyperTextParent(do_QueryObject(GetParent()));
or
nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible));
[I wonder whether we should have this in xpcom/glue somewhere?]

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Neil, when I compile the patch then I get an error:

/builds/slave/sendchange-linux-hg/build/accessible/src/base/nsApplicationAccessible.cpp: In member function ‘virtual nsresult nsApplicationAccessible::AddRootAccessible(nsIAccessible*)’:
/builds/slave/sendchange-linux-hg/build/accessible/src/base/nsApplicationAccessible.cpp:210: error: conversion from ‘nsQueryObject<nsIAccessible>’ to non-scalar type ‘nsRefPtr<nsAccessible>’ requested

It sounds the overloaded cast operator to already_addRefed<U> is not enough. Do you have any idea what can be wrong?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Neil, when I compile the patch then I get an error:
> 
> /builds/slave/sendchange-linux-hg/build/accessible/src/base/nsApplicationAccessible.cpp:
> In member function ‘virtual nsresult
> nsApplicationAccessible::AddRootAccessible(nsIAccessible*)’:
> /builds/slave/sendchange-linux-hg/build/accessible/src/base/nsApplicationAccessible.cpp:210:
> error: conversion from ‘nsQueryObject<nsIAccessible>’ to non-scalar type
> ‘nsRefPtr<nsAccessible>’ requested
> 
> It sounds the overloaded cast operator to already_addRefed<U> is not enough. Do
> you have any idea what can be wrong?

It's on Linux, no error on Windows. Ginn, do you ideas?

Comment 4

[Ginn Chen]

It works if I use
nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible));

nsRefPtr<nsAccessible> rootAcc = do_QueryObject(aRootAccessible);
would call nsRefPtr<T>'s constructor.
We didn't implement a constructor for nsRefPtr<T> from nsQueryObject<U>&.
If you want to make it work, you need to
1) Remove already_AddRefed<U>() operator for nsQueryObject<T>
2) Implement a constructor for nsRefPtr<T> from nsQueryObject<U>&.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> It works if I use
> nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible));
> 
> nsRefPtr<nsAccessible> rootAcc = do_QueryObject(aRootAccessible);
> would call nsRefPtr<T>'s constructor.
> We didn't implement a constructor for nsRefPtr<T> from nsQueryObject<U>&.
> If you want to make it work, you need to
> 1) Remove already_AddRefed<U>() operator for nsQueryObject<T>
> 2) Implement a constructor for nsRefPtr<T> from nsQueryObject<U>&.

Both nsCOMPtr and nsRefPtr have constructors taking already_AddRefed<>. So I hoped nsQueryObject will be casted to already_AddRefed<> automatically when 
1) nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible));
2) nsRefPtr<nsAccessible> rootAcc = do_QueryObject(aRootAccessible);
3) rootAcc = do_QueryObject(aRootAccessible);

The only one way is to add constructors and overload operator = for nsCOMPtr and nsRefPtr, do I understand right?

Comment 6

[Ginn Chen]

Add constructors would be enough if you remove already_AddRefed<U>() operator in nsQueryObject.

If you keep already_AddRefed<U>() operator in nsQueryObject, you have to explicitly add operator = for nsCOMPtr and nsRefPtr.

Comment 7

[[e-mail]]

For 3) to work don't you need an assignment operator anyway?

Comment 8

[[e-mail]]

Although I still can't see why writing
t = v;
won't use v's operator u() and t's operator=(u&)
but writing
T t(v);
will use v's operator u() and t's constructor t(u&)

Comment 9

[Ginn Chen]

1) and 3) will just work with current patch.

1) and 2) will work if you add constructor for nsRefPtr.
But 3) will break, because of overloading ambiguity between
nsRefPtr(<nsAccessible>::operator=(const nsRefPtr<nsAccessible>*) and
"nsRefPtr<nsAccessible>::operator=(const alreadyAddRefed<nsAccessible>&)".

If you remove already_AddRefed<U>() operator in nsQueryObject, all 1) 2) 3)
will work.

(In reply to comment #8)
> Although I still can't see why writing
> t = v;
> won't use v's operator u() and t's operator=(u&)

I think it will.
But T t = v won't;

> but writing
> T t(v);
> will use v's operator u() and t's constructor t(u&)

Comment 10

[[e-mail]]

The point is that we don't want to call AddRef or Release, so we must not use the operator=(const t&) operator. In particular we can't have t = v; turn into t = t(v); so if we can't use the operator=(const u&) operator to turn it into t = u(v); then we also need an operator=(const v&) operator.

[Where t = nsRefPtr<T>, u = already_AddRefed<U>, and v = do_QueryObject<V>]

Comment 11

[Ginn Chen]

ok, so we need to implement t's constructor for (v) and t's operator=(const v&).

For
1) T t(v)
It will use t's constructor for v.
2) T t = v;
It will use t's constructor for v.
3) T t; t=v;
It will use t's operator=(const v&).

There's no use case for
v's operator() for u.
Do we still need it?

Comment 12

[[e-mail]]

Attachment: Possible patch

To save adding yet more constructors and assignment operators to nsCOMPtr, I leveraged the existing nsCOMPtr_helper mechanism to work with nsRefPtr.

Comment 13

[alexander :surkov (:asurkov)]

I tried the following cases

  nsCOMPtr<nsIAccessible> rootAccessible = aRootAccessible;
  nsRefPtr<nsAccessible> acc1(do_QueryObject(aRootAccessible));
  nsRefPtr<nsAccessible> acc2 = do_QueryObject(aRootAccessible));
  nsRefPtr<nsAccessNode> acc3(do_QueryObject(rootAccessible));
  nsRefPtr<nsAccessNode> acc4 = do_QueryObject(rootAccessible));
  acc1 = do_QueryObject(acc3);

  nsCOMPtr<nsIAccessible> acc5(do_QueryObject(aRootAccessible));
  nsCOMPtr<nsIAccessible> acc6 = do_QueryObject(aRootAccessible);
  nsCOMPtr<nsIAccessNode> acc7(acc1);
  nsCOMPtr<nsIAccessNode> acc6 = do_QueryObject(acc1);
  acc5 = do_QueryObject(acc7);

no compilation errors. So it works.

Neil, do you want to ask for reviews?

Comment 14

[alexander :surkov (:asurkov)]

Neil, ping.

Comment 15

[[e-mail]]

Sorry, too many reviews on bug 377319 ;-)

Comment 16

[[e-mail]]

Comment on attachment 421693 [details] [diff] [review]
Possible patch

Hmm, what sort of tests can I write for this? I guess I could take TestCOMPtr.cpp and s/COM/Ref/ and s/\<I// throughout.

Comment 17

[[e-mail]]

Attachment: With tests

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 422515 [details] [diff] [review]
With tests

Why is this called do_QueryObject if it calls QueryInterface?

Second, and more seriously, this looks to me like it's disguising an unsafe operation as a safe one.  Given class A : public nsIFoo and class B : public nsIFoo, this lets you do nsRefPtr<A>(do_QueryObject(p)) and get the nsIFoo* cast to an A* even if the actual object is a B.

Third, it doesn't even look like it does *that* correctly if a static_cast from nsIFoo* to A* requires a pointer offset (which it generally does if nsIFoo is not the first base class of A).

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #18)
> Third, it doesn't even look like it does *that* correctly if a static_cast from
> nsIFoo* to A* requires a pointer offset (which it generally does if nsIFoo is
> not the first base class of A).

Er, I see it does this in the nsRefPtr part, so never mind that.


However, I still think this makes the operation look safer than it ought to.  What it's doing is essentially a static_cast, so it should look like one, so that people reading the code know it's an operation that they (the reader) need to check is safe.


If you want to avoid the extra reference counting, I could see the value in a function for static_cast-ing an already_AddRefed to a derived class, containing static_cast in the function name.

Comment 20

[[e-mail]]

(In reply to comment #18)
> (From update of attachment 422515 [details] [diff] [review])
> Why is this called do_QueryObject if it calls QueryInterface?
Because I was worried that overloading do_QueryInterface would be inefficient (writing nsCOMPtr<nsIFoo> foo(do_QueryInterface(bar)); would use the template instead of the cheap function).

> Second, and more seriously, this looks to me like it's disguising an unsafe
> operation as a safe one.  Given class A : public nsIFoo and class B : public
> nsIFoo, this lets you do nsRefPtr<A>(do_QueryObject(p)) and get the nsIFoo*
> cast to an A* even if the actual object is a B.
> 
> Third, it doesn't even look like it does *that* correctly if a static_cast from
> nsIFoo* to A* requires a pointer offset (which it generally does if nsIFoo is
> not the first base class of A).
I was trying to generalise uses such as nsTreeBodyFrame::GetColumnImpl (which I just noticed should have been declared static). The idea is that A is supposed to have its own ID. (At one point it was possible to enforce that by making the ID private.) It would then be possible to replace nsRefPtr<nsTreeColumn> col = GetColumnImpl(aCol); with nsRefPtr<nsTreeColumn> col = do_QueryObject(aCol);

Comment 21

[[e-mail]]

If it helps, I could use T::GetCID() instead of NS_GET_TEMPLATE_IID(T)?

Comment 22

[[e-mail]]

Attachment: Using CIDs instead of IIDs

Now requires a CID to assign to an nsRefPtr.

Comment 23

[alexander :surkov (:asurkov)]

David, friendly ping. This is really nice thing for accessibility.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I guess my hesitation here is really that I'm not sure this is something we want to encourage by making it easier to do.  Within an XPCOM module, should we really be encouraging use of QueryInterface for two different things (interface support and concrete class testing) or should we be encouraging people to split those two functions up (as Frames currently do)?

Comment 25

[alexander :surkov (:asurkov)]

I don't think do_QueryFrame approach is suitable for accessibility since accessible objects are XPCOM objects and we need to query accessible objects from XPCOM interfaces and vise versa. However we could implement new internal interfaces for accessible objects so that we can keep nsCOMPtr usage. Is this a thing you keep in mind?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 422726 [details] [diff] [review]
Using CIDs instead of IIDs

I'd like to know if Benjamin thinks this sort of use of QueryInterface is something we should encourage (by adding this functionality) or discourage (by rejecting it).

Comment 27

[[e-mail]]

Some other potential uses of this sort of QueryInterface:

callers of nsTreeBodyFrame::GetColumnImpl, e.g.
>nsRefPtr<nsTreeColumn> col = GetColumnImpl(aCol);
becomes
>nsRefPtr<nsTreeColumn> col = do_QueryObject(aCol);

nsStandardUrl::Equals i.e.
>nsRefPtr<nsStandardURL> other;
>nsresult rv = unknownOther->QueryInterface(kThisImplCID,
>                                           getter_AddRefs(other));
becomes
>nsresult rv;
>nsRefPtr<nsStandardURL> other(do_QueryObject(unknownOther, &rv));

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 422726 [details] [diff] [review]
Using CIDs instead of IIDs

r=dbaron on the code.

Did you mean to add TestRefPtr to the makefile.

I'd still like you to get benjamin's approval on the concept before you land; I don't really like it, but I can see that it could be useful for some callers.

Comment 29

[Benjamin Smedberg]

Comment on attachment 422726 [details] [diff] [review]
Using CIDs instead of IIDs

Hrm. There are certainly cases where we have allowed people to QI directly to implementation classes. However, In those cases we used pseudo-IIDs (STATIC_IID_ACCESSOR), not CID accessors. It makes me feel funny that we're overloading CIDs as an XPCOM registration mechanism as well as a query mechanism.

Is there a compelling reason to use _CID_ACCESSOR instead of _IID_ACCESSOR, especially because we've tuned the IID_ACCESSOR to use static data instead of inline functions, which is a codesize win.

Given what I'm thinking now, I'd get rid of STATIC_CID_ACCESSOR entirely.

Comment 30

[[e-mail]]

Attachment: Back to IIDs

Also updated for bitrot, and restoring the missing tests Makefile.in change.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 439935 [details] [diff] [review]
Back to IIDs

ok, r=dbaron

Comment 32

[[e-mail]]

Pushed changeset 7a42d6fde7e6 to mozilla-central.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given that what this does is call QueryInterface, I think this should be renamed to do_QueryInterface.  I think it's causing a confusion (e.g., in attachment 443812 [details] [diff] [review] on bug 239008), where people are choosing to use do_QueryObject because of what they're querying *from* rather than what they're querying *to*.  And in any case having a distinction in names in the helpers is silly when the underlying method has the same name and there's no distinction underneath.

Comment 34

[[e-mail]]

(In reply to comment #33)
> I think it's causing a confusion (e.g., in
> attachment 443812 [details] [diff] [review] on bug 239008), where people are choosing to use
> do_QueryObject because of what they're querying *from* rather than what they're
> querying *to*.
do_QueryObject was intended to cover all the cases where do_QueryInterface does not apply, so yes, it works with object source and/or destination. It's unfortunate that a mistake in my implementation means that do_QueryInterface now actually works for an object target.

> And in any case having a distinction in names in the helpers is
> silly when the underlying method has the same name and there's no distinction
> underneath.
do_QueryObject is a template; do_QueryInterface is not. I'm not sure that do_QueryObject can be optimised as well as do_QueryInterface can.

Comment 35

[Benjamin Smedberg]

Hrm, I'm not sure that querying *from* a non-nsISupports is a particularly interesting case: all of these objects can be cast down to an nsISupports once you resolve ambiguity. And at that point, you don't need to template the nsQueryInterface class, right? Just add a helper for nsQueryInterface + nsRefPtr?

Comment 36

[[e-mail]]

(In reply to comment #35)
> Hrm, I'm not sure that querying *from* a non-nsISupports is a particularly
> interesting case: all of these objects can be cast down to an nsISupports once
> you resolve ambiguity.
Sorry, but I don't see how you resolve ambiguity, except by templating.

Comment 37

[Benjamin Smedberg]

The caller static_cast<nsIFoo*>