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

Status: RESOLVED FIXED

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

Description

[Alex Vallat]

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.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Is this a known issue (did it come up during our meetup?)

Comment 2

[alexander :surkov (:asurkov)]

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?

Comment 3

[James Teh [:Jamie]]

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.

Comment 4

[Alex Vallat]

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.

Comment 5

[James Teh [:Jamie]]

(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.

Comment 6

[Alex Vallat]

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

Comment 7

[Alex Vallat]

Attachment: Test program to check for doc child of browser

Comment 8

[James Teh [:Jamie]]

(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.

Comment 9

[Alex Vallat]

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.

Comment 10

[James Teh [:Jamie]]

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

Comment 11

[James Teh [:Jamie]]

I'm investigating this.

Comment 12

[James Teh [:Jamie]]

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?

Comment 13

[James Teh [:Jamie]]

(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 14

[James Teh [:Jamie]]

Attachment: Bug 1403180: Fix StripHandlerFromOBJREF for VT_DISPATCH on Windows 7.

StripHandlerFromOBJREF shortens the OBJREF by sizeof(CLSID), so it needs to seek the stream back after tweaking the OBJREF.
Previously, this was done using a relative seek.
Unfortunately, for some reason I can't fathom on Windows 7, this doesn't work when marshaling for VT_DISPATCH.
The Seek call succeeds, but either does nothing or sets the stream position to a garbage value.
Instead, we now use an absolute seek, which seems to behave.
This was breaking IAccessible::accNavigate and AccessibleChildren on Windows 7.

Review commit: https://reviewboard.mozilla.org/r/191922/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191922/

Comment 15

[James Teh [:Jamie]]

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

[(No longer employed by Mozilla) Aaron Klotz]

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...

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f92422cf9bc
Fix StripHandlerFromOBJREF for VT_DISPATCH on Windows 7. r=aklotz

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0f92422cf9bc

Comment 19

[James Teh [:Jamie]]

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.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jim, would you consider this a must fix for 57? Thanks!

Comment 21

[Jim Mathies [:jimm]]

(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.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Improves e10s+a11y code, Beta57+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c81df7f7b71f