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




Disability Access APIs
9 years ago
9 years ago


(Reporter: MarcoZ, Assigned: MarcoZ)


({access, fixed1.9.1, verified1.9.0.7})

Windows XP
access, fixed1.9.1, verified1.9.0.7
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: Simple fix. We don't want to confuse screen readers into thinking we support UIA.)


(1 attachment)



9 years ago
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 
           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

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
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:

Comment 6

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

Comment 7

9 years ago
(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

9 years ago
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

9 years ago
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

9 years ago
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

9 years ago
(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

9 years ago
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

9 years ago
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:

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;
Assignee: nobody → marco.zehe
Flags: blocking1.9.1?
Whiteboard: Simple fix. We don't want to confuse screen readers into thinking we support UIA.

Comment 14

9 years ago
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

9 years ago
Created attachment 357317 [details] [diff] [review]
Attachment #357317 - Flags: review?(aaronleventhal)


9 years ago
Attachment #357317 - Flags: review?(aaronleventhal) → review+

Comment 16

9 years ago
Comment on attachment 357317 [details] [diff] [review]

Marco, which ATs did you test with?

Comment 17

9 years ago
Need to update http://developer.mozilla.org/en/docs/Accessibility:AT-APIs
Keywords: dev-doc-needed

Comment 18

9 years ago
Pushed in changeset:
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 19

9 years ago
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

9 years ago
You know what, don't worry about it. This will be part of the IA2 docs.
Keywords: dev-doc-needed

Comment 21

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

Comment 22

9 years ago
(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:

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.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3

Comment 23

9 years ago
Pushed to 1.9.1 branch in changeset:
Keywords: fixed1.9.1


9 years ago
Attachment #357317 - Flags: approval1.9.0.7?

Comment 24

9 years ago
Comment on attachment 357317 [details] [diff] [review]

No risk, just makes sure we're fully MSAA compliant with this particular function call.
Comment on attachment 357317 [details] [diff] [review]

Approved for, a=dveditz for release-drivers.
Attachment #357317 - Flags: approval1.9.0.7? → approval1.9.0.7+

Comment 26

9 years ago
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
Keywords: fixed1.9.0.7

Comment 27

9 years ago
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv: Gecko/2009021006 GranParadiso/3.0.7pre
Keywords: fixed1.9.0.7 → verified1.9.0.7

Comment 28

9 years ago
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:

Comment 29

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


9 years ago
Depends on: 487615
You need to log in before you can comment on or make changes to this bug.