Closed Bug 1407475 Opened 6 years ago Closed 6 years ago

IAccessible::accNavigate(NAVRELATION_EMBEDS) returns E_FAIL with e10s enabled

Categories

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

Product:
Core
Component:
Disability Access APIs
Platform:
All
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- disabled
firefox56 --- disabled
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

Details

Attachments

(1 file)

Bug 1407475: Fix IAccessible::accNavigate(NAVRELATION_EMBEDS) for e10s.
59 bytes, text/x-review-board-request
surkov
: review+
ritu
: approval-mozilla-beta+
Details 
Some a11y clients use IAccessible::accNavigate with NAVRELATION_EMBEDS to get the document in the current tab, as recommended by MDN. Firefox 56 regressed this for single process such that it returns E_NOTIMPL. This was fixed in bug 1405065 for single process. However, this allows a multi-process specific bug to manifest. With multi-process, accNavigate(NAVRELATION_EMBEDS) returns E_FAIL.

Steps to reproduce:
Get IAccessible object with AccessibleObjectFromWindow(OBJID_CLIENT);
Try to get web content's IAccessible object with IAccessible::accNavigate(NAVRELATION_EMBEDS).

Actual results:
IAccessible::accNavigate(NAVRELATION_EMBEDS) returns E_FAIL.

Expected results:
Should return 0 and retrieve the IAccessible object.

Simple testing instructions with the NVDA Python console:
1. Switch to the Firefox window you want to test.
2. Open the NVDA Python console by pressing NVDA+control+z.
3. Enter this command:
fg.IAccessibleObject.accNavigate(0x1009, 0)
If it succeeds, you should get a Dispatch object back; e.g. <comtypes.client.lazybind.Dispatch object at 0x05185A50>
If it fails, you'll get a traceback.
Assignee: nobody → jteh
Tested manually:
1. With e10s, verified that NAVRELATION_EMBEDS on the root works as expected, including switching between multiple tabs.
2. Verified 1) but with a non-e10s window.
3. Verified 1) but with e10s disabled.
4. Verified that NAVDIR_FIRSTCHILD still works on the root (since this code path has changed).
Comment on attachment 8917246 [details]
Bug 1407475: Fix IAccessible::accNavigate(NAVRELATION_EMBEDS) for e10s.

https://reviewboard.mozilla.org/r/188262/#review194110

::: accessible/windows/msaa/RootAccessibleWrap.cpp:117
(Diff revision 1)
> +  // Special handling for NAVRELATION_EMBEDS.
> +  // When we only have a single process, this can be handled the same way as
> +  // any other relation.
> +  // However, for multi process, the normal relation mechanism doesn't work
> +  // because it can't handle remote objects.
> +  if (NAVRELATION_EMBEDS != navDir

nit: I would swap the ordering:
if (navDir != NAVRELATION_EMBEDS)
it seems more natural with me :)

::: accessible/windows/msaa/RootAccessibleWrap.cpp:119
(Diff revision 1)
> +  // any other relation.
> +  // However, for multi process, the normal relation mechanism doesn't work
> +  // because it can't handle remote objects.
> +  if (NAVRELATION_EMBEDS != navDir
> +    || VT_I4 != varStart.vt || CHILDID_SELF != varStart.lVal
> +  ) {

nit: 
if (cond1 ||
    cond2) {
}

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: accessible/windows/msaa/RootAccessibleWrap.cpp:122
(Diff revision 1)
> +  if (NAVRELATION_EMBEDS != navDir
> +    || VT_I4 != varStart.vt || CHILDID_SELF != varStart.lVal
> +  ) {
> +    // We only handle EMBEDS on the root here.
> +    // Forward to the base implementation.
> +    return AccessibleWrap::accNavigate(navDir, varStart, pvarEndUpAt);

Feels safer to redirect to DocAccessibleWrap (direct base class in hierarchy), even if there's no implementation, otherwise it makes me think that AccessibleWrap was used on purpose to bypass DocAccessibleWrap for some reason.

::: accessible/windows/msaa/RootAccessibleWrap.cpp:130
(Diff revision 1)
> +  if (!pvarEndUpAt) {
> +    return E_INVALIDARG;
> +  }
> +
> +  Accessible* target = nullptr;
> +  ProxyAccessible* docProxy = GetPrimaryRemoteTopLevelContentDoc();

it might be nice to have a small comment that we grab a tab document here
Attachment #8917246 - Flags: review?(surkov.alexander) → review+
Keywords: checkin-needed
Attachment #8917246 - Flags: checkin?
Comment on attachment 8917246 [details]
Bug 1407475: Fix IAccessible::accNavigate(NAVRELATION_EMBEDS) for e10s.

For future reference, you can set just the checkin-needed keyword when you want a patch landed. The checkin? flag doesn't work as well with our automated bug marking tools, so manual intervention ends up being needed.
Attachment #8917246 - Flags: checkin?
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e0f3918c6e30
Fix IAccessible::accNavigate(NAVRELATION_EMBEDS) for e10s. r=surkov
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0f3918c6e30
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Note if this get's uplifted we'll need the crash fix from bug 1408638 as well.
Comment on attachment 8917246 [details]
Bug 1407475: Fix IAccessible::accNavigate(NAVRELATION_EMBEDS) for e10s.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility implementation.
[User impact if declined]: Breaks accessibility clients that use NAVRELATION_EMBEDS to access the tab document. Note that this technique is prominently documented on MDN, so it's reasonable that a11y clients would use it. Several people report they are affected by this; see related bug 1405065, comments 0, 15, 17.
[Is this code covered by automated tests?]: No; we don't have a mechanism for platform a11y testing.
[Has the fix been verified in Nightly?]: I've verified myself on Nightly, but this hasn't been confirmed by anyone else yet.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: Follow up crash fix in bug 1408638; https://bugzilla.mozilla.org/attachment.cgi?id=8918704.
[Is the change risky?]: no
[Why is the change risky/not risky?]: Low risk. Only touches one a11y method and thus should at worst only affect that method. However, it's worth noting that one crash was identified (and fixed) where a client called this method in an unexpected way; see bug 1408638.
[String changes made/needed]: none
Attachment #8917246 - Flags: approval-mozilla-beta?
Gindi, Rajesh, Robinson, would one of you be able to verify that this is fixed for multi process in latest nightly? Thanks.
Verified with FF nightly build 58.0a1 (2017-10-16) (32-bit) and it is working fine with multi-process.
Firefox Nightly 58.0a1 (2017-10-16) (64-bit)

Test conditions:
Multi-process enabled.
Nigtly started with several tabs.

Test A: Testing with the startup tab page.
Results:
1. IAccessible::accNavigate(NAVRELATION_EMBEDS) returns 0x80004005 (E_FAIL).
2. AccessibleObjectFromPoint does not get web page objects. First time it gets object of role ROLE_SYSTEM_APPLICATION, then always "browser".
3. Cannot find a web page object by walking the MSAA object tree.
4. Inspect.exe shows that there are no MSAA and UI Automation objects in web page.
Trying several times does not help.

Test B: Switch to another tab.
Results:
1. IAccessible::accNavigate(NAVRELATION_EMBEDS) still returns 0x80004005 (E_FAIL). Once returned 0 first time.
2. AccessibleObjectFromPoint now gets correct object (link etc).
3. Can find a web page object by walking the MSAA object tree.
4. Inspect.exe shows MSAA and UI Automation objects in web page.

Test C: Swith back to the startup tab.
Results:
The same as Test B.

------

But sometimes everything start to work, even without switching tabs. Then even IAccessible::accNavigate(NAVRELATION_EMBEDS) returns 0. It somehow happens after various tests with various tools, but now I'm unable to reproduce intentionally. After restarting Nightly, does not work again.
More info.
It starts to work after about a minute of calling accNavigate(NAVRELATION_EMBEDS) or AccessibleObjectFromPoint repeatedly, for example every 1 s. After the first call, Firefox shows the "Accessibility Features Enabled" icon. But enables only after 30-60 calls.
Gindi, what version of Windows are you testing with? There is another bug related to Windows 7 that might explain the results you're seeing. However, this should work as expected in Windows 10.
Windows 10 64-bit, build 15063.674.
Didn't test Nightly on Windows 7.
Tested Firefox Nightly 58.0a1 (2017-10-16) (64-bit) on Windows 8.1 Pro (64-bit) machine with Multi-process mode, result is: "accNavigate" returns "E_POINTER", and not able to find web page objects.

Code snippet is given below:

IAccessible *pAccBrowser = NULL;
hr = AccessibleObjectFromWindow(hwnd, OBJID_CLIENT, IID_IAccessible, (void**)&pAccBrowser); //SUCCESS
hr = pAccBrowser->accNavigate(NAVRELATION_EMBEDS,vtStart,&vtResult); // FAILS, return value -> E_POINTER
Same for me, Works fine with  Windows 10 and failed on Windows 7, 8
This sequence of actions also makes it to work:
1. Call accNavigate or AccessibleObjectFromPoint while Firefox window is not the active window. Fails.
2. Activate Firefox window. Then accNavigate/AccessibleObjectFromPoint already works.

or

1. Call accNavigate/AccessibleObjectFromPoint while Firefox window is active.
2. Make another window active.
3. Activate Firefox window. Then accNavigate/AccessibleObjectFromPoint already works.

-----

BTW, works immediately if NVDA is running when Firefox starts.
Comment on attachment 8917246 [details]
Bug 1407475: Fix IAccessible::accNavigate(NAVRELATION_EMBEDS) for e10s.

e10s a11y fix for win10 that was verified on nightly, Beta57+
Attachment #8917246 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.