458871 - QueryService always returns an object while it should check service ID.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[Marco Zehe (:MarcoZ)]

Description from Masahiko from the Microsoft MSAA team:
QueryService on Firefox IAccessible objects apparently doesn't check the service ID, so always returns an object. It should check specific service ID for
returning associated interface object. 
  
The IServiceProvider::QueryService implementation should be returning E_INVALIDARG if it doesn’t support the requested service specifically. 
  
The pseudo-code might be something like this: 
    IServiceProvider::QueryService( REFGUID guidService, REFIID riid, void **ppv ) 
       If guidService == service1 
           Return a reference to service1 object 
       Else if guidService == service2 
           Return a reference to service2 object 
       Else 
           Return E_INVALIDARG 
  
With Firefox’s implementation, we can send in any value for guidService and it returns a reference to some object, which is very unexpected.  It makes
it impossible to find out if a service is unsupported.

Comment 1

[alexander :surkov (:asurkov)]

Who use this and what do they pass as guidService? Aaron or Marco, could you know please? (for the reference reference http://msdn.microsoft.com/en-us/library/ms696078(VS.85).aspx). Should we use different guidServices for IID_IAccessibleApplication and ISimpleDOM interfaces or IAccessible2 interfaces?

Comment 2

[Aaron Leventhal]

Maybe David or Pete know. Mashiko isn't answering.

Otherwise, we need to find out how the resident COM expert is in Mozilla land.

Comment 3

[Pete Brunet]

I'm not sure what the question is but QS should allow access to all the supported interfaces (like QI) and return an error code if the requested interface is not supported.

Comment 4

[Aaron Leventhal]

Pete, what about the Service ID? What should we ask callers to pass in for that parameter? Masahiko from Microsoft says we need to check it for proper QueryService usage.

Comment 5

[Pete Brunet]

My apologies.  I was talking about the interface ID.  At http://accessibility.freestandards.org/a11yspecs/ia2/docs/html/_generalinfo.html#_dicoveringInterfaces I have an example which indicates that the service ID is the GUID of the IAccessible interface.  I don't remember who gave me that example.  All the other cases I found via Google don't use the same service guid as one of the interfaces, but 

  // pAcc is a reference to the accessible object's IAccessible interface. 
  IServiceProvider *pService = NULL; 
  hr = pAcc->QueryInterface(IID_IServiceProvider, (void **)&pService); 
  if(SUCCEEDED(hr)) { 
    IAccessible2 *pIA2 = NULL; 
    hr = pService->QueryService(IID_IAccessible, IID_IAccessible2, (void**)&pIA2); 
    if (SUCCEEDED(hr) && pIA2) { 
      // The control supports IAccessible2. 
      // pIA2 is the reference to the accessible object's IAccessible2 interface.
    } 
  } 

Looking at various examples, I haven't found good guidance on how the SID is supposed to be used.  Maybe you can figure out something from these links:
http://msdn.microsoft.com/en-us/library/ms528415.aspx#acc_obj
http://codecentrix.blogspot.com/2007/10/from-iaccessible-to-ihtmlelement-and.html

Comment 6

[Aaron Leventhal]

Mick, Jamie? What do you send for the service ID to QueryService?

Comment 7

[Michael Curran]

(In reply to comment #6)
> Mick, Jamie? What do you send for the service ID to QueryService?
We pass IID_IAccessible as the first param, and IID_IAccessible2 as the second param.
We follow exactly what it says in accessible2.idl.

However, there is another paragraph in accessible2.idl that states that queryService can be used to also switch between IAccessible2 interfaces. I have no idea exactly what to provide for that, NVDA always uses queryInterface between IAccessible2 interfaces.

Comment 8

[Aaron Leventhal]

No one seems to know how this would be useful.

But, at least it seems like most ATs pass IID_IAccessible. I'm a bit afraid that some don't. So, the work for this bug is actually to check with everyone. The code would be a 2 line fix.

if (service != IID_IAccessible)
  return E_INVALIDARG;

Comment 9

[Michael Curran]

I have asked the IA2 community to consider better standardizing the way QueryService is used in IA2 applications.
However until this has been decided upon, I think that accepting services of IID_IAccessible, IID_IAccessible2 and what ever ISimpleDom uses.
The second parameter (the interface) technically should only be allowed to be an interface the service supports (e.g. IAccessible2 would allow IAccessibleText). But I guess for now it should be able to accept anything.

This is an extremely important bug for NVDA as it starts to implement support for UI Automation. ATs and accessibility bridges/proxies and the like depend on QueryService returning an error for services it doesn't support. I.E. if some kind of fictional bridge asks Mozilla if it supports the fictionalX service, currently Mozilla would say yes, and the bridge would then try using it and no doubt crash itself and the Mozilla application.

Comment 10

[Aaron Leventhal]

Mick, I must be missing something. We would return a failure for any interface we don't support. So on an IID basis, if you pass in a UI Automation interface, you'd get a failure.

Why isn't that enough?

Comment 11

[Michael Curran]

(In reply to comment #10)
> Mick, I must be missing something. We would return a failure for any interface
> we don't support. So on an IID basis, if you pass in a UI Automation interface,
> you'd get a failure.
> 
> Why isn't that enough?
Sorry, I probably over explained the problem.
In short QueryService must give back an object for any service/interface paire it supports. It must return an error for any service/interface paire it doesn't.
I was just thinking for ease of coding that only the service needs to be checked. The interface could also be checked, but technically that will be *all* the IAccessible2 interfaces, plus all the ISimpleDom interfaces, plus IAccessible.

Comment 12

[Aaron Leventhal]

Ok, I get it now, the problem is if someone does
QueryService(IID_UIA, IUnknown) or
QueryService(IID_UIA, IDispatch)
should not succeed

We should return failure unless the service ID is one of the following:
IID_ISimpleDOMNode, IID_IAccessible, IID_IAccessible2

Comment 13

[Aaron Leventhal]

Also IID_IAccessibleApplication

Marco, we will need to test with all the Windows screen readers that support us. Can I assign it to you? It's just a one line patch here:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/nsAccessNodeWrap.cpp#138

Just add this to the top of the method:
if (guidService != IID_ISimpleDOMNode && guidService != IID_IAccessible && 
    guidService != IID_IAccessible2 && guidService != IID_IAccessibleApplication) 
  return E_INVALIDARG;

Comment 14

[Aaron Leventhal]

It looks like there was an older IID that some ATs still use for that argument. Apparently it came from ISimpleDOMNode.idl but I couldn't find the original source. At least JAWS and NVDA are using it (in NVDA's case I only see it in some debugging code).

So, we need to define the guid something like this:
static const GUID IID_SimpleDOMDeprecated = {0x0c539790, 0x12e4, 0x11cf, 0xb6, 0x61, 0x00, 0xaa, 0x00, 0x4c, 0xd6, 0xd8};
Or try using the DEFINE_GUID macro, but I don't know what to #include to get that, or if you already have it:
DEFINE_GUID(IID_SimpleDOMDeprecated, 0x0c539790, 0x12e4, 0x11cf, 0xb6, 0x61, 0x00 , 0xaa, 0x00, 0x4c, 0xd6, 0xd8)

Then add it to your if statement:
if (guidService != IID_ISimpleDOMNode && guidService != IID_SimpleDOMDeprecated &&
    guidService != IID_IAccessible && guidService != IID_IAccessible2 &&
    guidService != IID_IAccessibleApplication) 
  return E_INVALIDARG;

Try something like that.

Comment 15

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Comment 16

[Aaron Leventhal]

Comment on attachment 357317 [details] [diff] [review]
Patch

Marco, which ATs did you test with?

Comment 17

[Aaron Leventhal]

Need to update http://developer.mozilla.org/en/docs/Accessibility:AT-APIs

Comment 18

[Marco Zehe (:MarcoZ)]

Pushed in changeset:
http://hg.mozilla.org/mozilla-central/rev/0a6dd4ba5a3b

Comment 19

[Marco Zehe (:MarcoZ)]

Aaron, the URL above does not specifically mention queryService (it doesn't mention any query* method at all, for that matter), and also no IIDs. Also the MSAA specific article linked to does not cite any of this. Where should this info go?

Comment 20

[Aaron Leventhal]

You know what, don't worry about it. This will be part of the IA2 docs.

Comment 21

[Pete Brunet]

Is the convention to use IID_* for service IDs?  Or SID_*?

Comment 22

[Michael Curran]

(In reply to comment #21)
> Is the convention to use IID_* for service IDs?  Or SID_*?
I found one document in google mentioning that SID_ should be used. However, the most common QueryService from MSAA existing before IAccessible2 was in Internet Explorer to get an IHTMLElement from an IAccessible.
To do this you pass QueryService IID_IHTMLElement for both the service and interface IDs.
A URL that shows this is:
http://msdn.microsoft.com/en-us/library/ms528415%28VS.85%29.aspx

I guess it wouldn't be bad if we had an SID_IAccessible2 or SID_IAccessibleApplication constant, though I don't think there is an SID_IAccessible, nore would older code support this.
It's probably best to just use IID_IAccessible2 or what Gecko has always been using which is IID_IAccessible.

Comment 23

[Marco Zehe (:MarcoZ)]

Pushed to 1.9.1 branch in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9c2395c106d8

Comment 24

[Marco Zehe (:MarcoZ)]

Comment on attachment 357317 [details] [diff] [review]
Patch

No risk, just makes sure we're fully MSAA compliant with this particular function call.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 357317 [details] [diff] [review]
Patch

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 26

[Marco Zehe (:MarcoZ)]

Checking in accessible/src/msaa/nsAccessNodeWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessNodeWrap.cpp,v  <--  nsAccessNodeWrap.cpp
new revision: 1.47; previous revision: 1.46
done

Comment 27

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.7pre) Gecko/2009021006 GranParadiso/3.0.7pre

Comment 28

[Pete Brunet]

Sorry for the late response here.  My transition out of IBM had me a bit sidetracked.

For the IA2 spec we are considering the following requirement when QSing an IAccessible to get one of the IA2 interfaces:
If the SID is IID_IAccessible2 and the IID is IID_IUnknown or any of the IA2 interfaces then return the requested interface, else return E_NOINTEFACE.

Since IUnknown is an interface on an object implementing IA2 is there any need to be able to QS to it?

Also, should the QS of IAccessible only respond to IID_IAccessible2 or should it respond to any of the IA2 IIDs?

BTW, if it matters, although the IA2 IDL shows that IA2 derives from IA this is an error and should not be relied on and we should change it in some future version of IA2.

The full IA2 QI/QS spec proposal is at:
https://lists.linux-foundation.org/pipermail/accessibility-ia2/2009-February/000739.html

Comment 29

[Pete Brunet]

Also, Please see item 10 of this week's IA2 agenda at http://www.linuxfoundation.org/en/Accessibility/IAccessible2/Agenda/20090303