Calling AccessibleChildren on the browser element results in E_FAIL when using the AccessibleHandler

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bugzilla, Assigned: Jamie)

Tracking

Trunk
mozilla58
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925150345

Steps to reproduce:

Obtain the IAccessible element for the browser. Starting at the top level Window element, child elements through the hierarchy can be obtained with the roles:

Window
 Application
  Grouping
    PropertyPage
     browser


Actual results:

This browser element reports having one child to the get_accChildCount method, however when the AccessibleHandler is installed (COM-registered) calling AccessibleChildren on the browser element returns E_FAIL. Similarly, calling .accNavigate(NAVDIR_FIRSTCHILD, 0) on the browser element returns E_FAIL.


Expected results:

Without the AccessibleHandler present, the browser element correctly returns a child with the Document accRole, whose accValue is the URL of the page being displayed.
(Reporter)

Updated

2 years ago
OS: Unspecified → Windows 7
Hardware: Unspecified → x86
(Reporter)

Updated

2 years ago
Hardware: x86 → x86_64
Component: Untriaged → Disability Access APIs
Product: Firefox → Core
Is this a known issue (did it come up during our meetup?)
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(mzehe)
I'm pretty sure that I saw in a bug log a parent(chrome)-to-child(tabdoc) navigational issue, and I recall Jamie commented on this.

Jamie, is this issue covered by some other bugs already, correct?
Flags: needinfo?(surkov.alexander) → needinfo?(jteh)
(Assignee)

Comment 3

2 years ago
I don't recall anything about this.

Alex V, is your client in-process or out-of-process? The handler shouldn't get used for out-of-process clients now (bug 1368571). I just want to make sure we're chasing the right problem.
Flags: needinfo?(jteh) → needinfo?(bugzilla)
(Reporter)

Comment 4

2 years ago
The client is out of process. I can reproduce it using inspect.exe (the child of PropertyPage is reported as "none: [error getting role] : focusable[error getting state] if selected in the tree will not populate the right-hand pane).

In my own (CLR-based) client code, I can reproduce the problem in both x64 and x86 builds.
Flags: needinfo?(bugzilla)
(Assignee)

Comment 5

2 years ago
(In reply to Alex Vallat from comment #4)
> I can reproduce it using inspect.exe (the
> child of PropertyPage is reported as "none: [error getting role] :
> focusable[error getting state] if selected in the tree will not populate the
> right-hand pane).

I get this too. However, I'm dubious about trusting this, since it says there was an error getting the role and comment 0 suggests you could retrieve the role just fine (as can I). More likely, inspect doesn't like the use of a string role here.

I can't reproduce the accNavigate or AccessibleChildren issues using Python and comtypes. I'm testing on Windows 10 x64, so perhaps this is somehow Windows 7 specific. I don't have a way to test that just yet.
(Reporter)

Comment 6

2 years ago
I've done some testing with Windows 10, and can confirm that this seems to be Windows 7 specific. Strangely, though, even on Windows 10 it does not work for the first tab. If I run Nightly, navigate to example.com and test, the doc child of browser is inaccessible (E_FAIL). If then I open a new tab, navigate to example.doc in the new tab, and close the first tab, then the doc child of the browser is accessible on Windows 10, and on Windows 7 only if AccessibleHandler.dll is unregistered.

I will attach a minimal test program in C# that I used to validate this.

Repro steps:

* Run Nightly
* Navigate to example.com
* Run test program => Result A
* Open new tab, navigate to example.com
* Close first tab
* Run test program => Result B

My results are:

Result A: Always fail.
Result B:

Win 10 + AccessibleHandler : Pass
Win 10 + No Handler        : Pass
Win 7  + Accessible Handler: Fail
Win 7  + No Handler        : Pass
(Assignee)

Comment 8

2 years ago
(In reply to Alex Vallat from comment #6)
> Strangely, though, even on Windows 10 it does not
> work for the first tab. If I run Nightly, navigate to example.com and test,
> the doc child of browser is inaccessible (E_FAIL).

thanks heaps for the test program. I can reproduce this, but:
1. The child count for the "browser" accessible in this case is 0. I confirmed this by putting this line just after the "var browser" line:
		Console.WriteLine("Browser child count: " + browser.accChildCount);
2. This happens with or without AccessibleHandler.dll registered.
3. Interestingly, this only happens if I load Firefox without letting my screen reader see it. That suggests some other a11y stuff (probably something to do with events) is triggering the creation of the document.

It'd be good if you could confirm points 1 and 2 above on Windows 10.

That then raises the question of whether this is the same issue or a different one. You originally noted that the child count was 1 and that this only happened when the handler wasn't registered. Do you ever see a 0 child count in Windows 7?

Sorry for all the questions. Just trying to narrow this down a little.
(Reporter)

Comment 9

2 years ago
Child count is always 0 for the Result A case in my previous comment, under both Windows 10 and 7. For the Result B case (after opening a new tab, navigating to example.com, and closing the original tab), the child count is 1. Under Windows 7 with the Accessible Handler registered, the child count is still 1, but attempting to navigate to that child fails.

So this is the same issue as initially described, it's just narrowed down to "Not on Windows 10", and "Not the initial tab immediately after running Firefox".

There's an arguably separate issue that the initial tab immediately after running Firefox does not not expose the Document child of the browser (but does have a consistent accChildCount of 0), and that is reproducible on both Windows 7 and 10.
(Assignee)

Comment 10

a year ago
I can confirm both cases of this issue. Regarding the case where the child count is 0, that's covered by bug 1329977.
(Assignee)

Comment 11

a year ago
I'm investigating this.
Assignee: nobody → jteh
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 12

a year ago
I've spent quite a lot of time debugging this and I'm not getting very far. :(

Here's what I know:
1. The issue occurs only on Windows 7 with the handler enabled. Windows 7 without the handler and Windows 10 with/without the handler work just fine.
2. When called out-of-process, IAccessible::accNavigate and IEnumVARIANT for content accessibles return E_FAIL. However, IAccessible::accParent and IAccessible::accChild work just fine.
3. All of accParent, accChild and accNavigate output IDispatch. However, accNavigate is the only one of these that does so using a VARIANT with VT_DISPATCH. IEnumVARIANT also uses VT_DISPATCH. This suggests this is somehow specific to VT_DISPATCH marshaling.
4. accNavigate(NAVRELATION_EMBEDS) in the parent process also fails. That suggests the same failure when marshaling VT_DISPATCH out of the parent.
5. NVDA is able to render tables in-process and it uses AccessibleChildren, so that suggests this works when the handler isn't stripped.
6. Therefore, it seems VT_DISPATCH fails when the handler is registered but stripped.
7. StripHandlerFromOBJREF succeeds, as does its caller, Interceptor::MarshalInterface.
8. After Interceptor::MarshalInterface returns, ole32!WdtpInterfacePointer_UserMarshal (called from VARIANT_UserMarshal) throws a code 80004005 exception (E_FAIL):
00 072bf01c 76672698 RPCRT4!RpcRaiseException
01 072bf07c 7609c4d9 ole32!WdtpInterfacePointer_UserMarshal(struct _USER_MARSHAL_CB * pContext = 0x038e2f18, unsigned long Flags = 0, unsigned char * pBuffer = 0x038e2f9c "???", struct IUnknown * pIf = 0x03922064, struct _GUID * IId = 0x760a0e48 {00020400-0000-0000-C000-000000000046})+0x88 [d:\w7rtm\com\ole32\oleprx32\proxy\transmit.cxx @ 1121] 
02 072bf0a4 75c34e60 OLEAUT32!VARIANT_UserMarshal+0x1a6

Aaron, any thoughts you could offer here would be greatly appreciated.

I see that StripHandlerFromOBJREF changes the objref type, zeroes out the handler clsid and then seeks back to before the clsid. Is it possible that this seeking back (with zero padding at the end of the stream) somehow causes problems for the VARIANT marshal in Win7; i.e. perhaps it checks the length of the stream or something? I'm totally just grabbing at straws here, though.

I also don't quite follow why we need to strip the handler after it's written. Why can't we just check IsCallerExternalProcess() in Interceptor::GetClassForHandler and return E_NOINTERFACE if true?
(Assignee)

Comment 13

a year ago
(In reply to James Teh [:Jamie] from comment #12)
> I also don't quite follow why we need to strip the handler after it's
> written. Why can't we just check IsCallerExternalProcess() in
> Interceptor::GetClassForHandler and return E_NOINTERFACE if true?

I just tried this and it seems to work when accNavigate is called on a content accessible. (I also had to modify GetMarshalSizeMax and marshalInterface to return early after calling the standard marshaler if IsCallerExternalProcess.) The handler does still get used in the parent process, though.

However, it's not so easy when remarshaling from the parent process, since marshaling a proxy from content of course gives us the handler. Unfortunately, here we hit the same problem when we try to use NAVRELATION_EMBEDS on the root accessible. Not sure what to do about that one. :(
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
So, I gave up on the GetClassForHandler idea. There's just no way I can find to get that to work in the parent process. As a long shot, I tried implementing IStdMarshalInfo in the handler and returning E_NOINTERFACE, but as I guessed, this doesn't work.

I started digging into StripHandlerFromOBJREF instead. As noted in my commit, IStream::Seek seems to behave weirdly on Windows 7 in some cases. Most of the time, the stream is 132 bytes and we then subtract 16 when we tweak the OBJREF and remove the CLSID, and this works just fine. With VT_DISPATCH, however, we see a call where the stream is 140 bytes. In this case, if you do the relative seek and then check the final position, you get an insanely large number. If you check the position before the seek, you get 140, but even though the seek succeeds, checking the position again after the seek gives you 140 again, as if the position hadn't been adjusted. Doing an absolute seek (by subtracting from the previous end position) seems to work just fine.

I'm a bit unhappy about hacking around this when I don't fully understand what's going on. My only thought was that something else might be messing around with the stream, but I don't think so; aside from not being able to fathom why this would be, the failure is 100% consistent. The fact that it works in Windows 10 does lead me to suspect a Windows bug, though.

Comment 16

a year ago
mozreview-review
Comment on attachment 8920954 [details]
Bug 1403180: Fix StripHandlerFromOBJREF for VT_DISPATCH on Windows 7.

https://reviewboard.mozilla.org/r/191922/#review198744

Bizarre! But hey, if it works...
Attachment #8920954 - Flags: review?(aklotz) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 17

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0f92422cf9bc
Fix StripHandlerFromOBJREF for VT_DISPATCH on Windows 7. r=aklotz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f92422cf9bc
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 19

a year ago
Comment on attachment 8920954 [details]
Bug 1403180: Fix StripHandlerFromOBJREF for VT_DISPATCH on Windows 7.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Some functionality in various accessibility clients will not work in Windows 7 for web documents. For example, NVDA object navigation does not work.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for automated testing of platform accessibility.
[Has the fix been verified in Nightly?]: I manually confirmed that this works as expected using NVDA on Windows 7.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Only affects e10s Windows accessibility, which is already partly broken without this patch anyway.
[String changes made/needed]: None.
Attachment #8920954 - Flags: approval-mozilla-beta?
Hi Jim, would you consider this a must fix for 57? Thanks!
Flags: needinfo?(jmathies)
(In reply to Ritu Kothari (:ritu) from comment #20)
> Hi Jim, would you consider this a must fix for 57? Thanks!

Looks safe to me, I think we should take it.
Flags: needinfo?(jmathies)
status-firefox57: --- → affected
Comment on attachment 8920954 [details]
Bug 1403180: Fix StripHandlerFromOBJREF for VT_DISPATCH on Windows 7.

Improves e10s+a11y code, Beta57+
Attachment #8920954 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c81df7f7b71f
status-firefox57: affected → fixed
You need to log in before you can comment on or make changes to this bug.