1564542 - Firefox crashes when using the Accessibility Inspector on the browser mozilla::a11y::Accessible::Bounds()

Status: VERIFIED FIXED

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

Description

[Asa Dotzler [:asa]]

Tested on:
Windows 10, 64-bit
Firefox Nightly 70.0a1 (2019-07-09) (64-bit)

Steps to reproduce:

1. Launch Firefox
2. Enable the browser toolbox https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
3. Open the browser toolbox
4. Select Accessibility in the toolbar
5. Select the Accessibility Inspector
6. Move the pointer over Firefox

Results:
Crash at mozilla::a11y::Accessible::Bounds() report at https://crash-stats.mozilla.org/report/index/4aebc748-07ec-4927-b986-6f6bd0190709

Expected results:
No crash

Comment 1

[Marcia Knous [:marcia]]

This looks to be the same crash as Bug 1512568 which was filed a while back.

Comment 2

[James Teh [:Jamie]]

I took a look at the crash dump. In the Bounds() call, this->mDoc is null. Furthermore, this->mType is 0x1f (eProxyType).

I think this is because on Windows, calling GetChildAt(0) on an OuterDocAccessible for a remote document returns a ProxyAccessibleWrap. That wrapper is only capable of returning a native COM interface; most calls (including Bounds) will fail.

I'd like to deal with this in OuterDocAccessible::ChildAtPoint, but we can't because Accessible::ChildAtPoint fetches grandchildren (children of the found direct child). Thus, OuterDocAccessible's ChildAtPoint doesn't get called. I can think of two possible solutions:

1. Have a Windows specific override of Bounds in DocAccessible which returns a 0 rectangle if IsProxy() is true. The 0 rectangle thing is kinda ugly though.
2. Have Windows specific code in Accessible::ChildAtPoint which checks if the child IsProxy(), and if so, skips it. The disadvantage of this is that we'd be calling IsProxy on every child it checks, even if it's not a document.

Comment 4

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #2)

1. Have a Windows specific override of Bounds in DocAccessible which returns a 0 rectangle if IsProxy() is true. The 0 rectangle thing is kinda ugly though.

We'd probably do this in DocProxyAccessiblewrap and RemoteIframeDocProxyAccessibleWrap.

Comment 5

[James Teh [:Jamie]]

I'm not going to be so useful at testing with a mouse. Asa, would you mind giving this try build a spin to see if it fixes the issue?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de1f28000048137ba482338c7b921d66b35419d4

Comment 6

[Asa Dotzler [:asa]]

(In reply to James Teh [:Jamie] from comment #5)

I'm not going to be so useful at testing with a mouse. Asa, would you mind giving this try build a spin to see if it fixes the issue?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de1f28000048137ba482338c7b921d66b35419d4

I've tested the try build and it does not crash. The Accessibility Inspector works on browser chrome. Yay!

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1564542: Add a dummy implementation of Bounds() on DocProxyAccessibleWrap/RemoteIframeDocProxyAccessibleWrap to prevent crashes when hit testing via XPCOM.

OuterDocAccessible can return a DocProxyAccessibleWrap or RemoteIframeDocProxyAccessibleWrap as a child.
Accessible::ChildAtPoint on an ancestor might retrieve this proxy and call Bounds() on it.
This will crash on a proxy, so we override it on these document proxies to do nothing.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf6d08f93d8d
Add a dummy implementation of Bounds() on DocProxyAccessibleWrap/RemoteIframeDocProxyAccessibleWrap to prevent crashes when hit testing via XPCOM. r=yzen

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/bf6d08f93d8d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta or can this ride the trains?

Comment 11

[James Teh [:Jamie]]

Comment on attachment 9078565 [details]
Bug 1564542: Add a dummy implementation of Bounds() on DocProxyAccessibleWrap/RemoteIframeDocProxyAccessibleWrap to prevent crashes when hit testing via XPCOM.

Beta/Release Uplift Approval Request

• User impact if declined: Crashes on Windows when moving the mouse over the browser after enabling the Accessibility Inspector in the Browser Toolbox.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial fix which affects a very specific code path only engaged (outside of tests) when using the Accessibility Inspector in the Browser Toolbox on Windows.
• String changes made/needed: None.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9078565 [details]
Bug 1564542: Add a dummy implementation of Bounds() on DocProxyAccessibleWrap/RemoteIframeDocProxyAccessibleWrap to prevent crashes when hit testing via XPCOM.

Fixes a crash on Windows when using the Accessibility Inspector. Approved for 69.0b7.

Comment 13

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-beta/rev/524bbb009aa8

Comment 14

[Alexandru Trif, Desktop QA [:atrif]]

Hello!

Reproduced the issue using Firefox 70.0a1 (20190709153742) on Windows 10x64.

Although while verifying this using Firefox 69.0b7 (20190722201635) and Firefox 70.0a1 (20190722214346) on Windows 10 I encountered bug 1568163, another crash with a different signature.
It is safe to close this bug as verified?

Comment 15

[James Teh [:Jamie]]

Ug. Thanks for catching; not sure why that one doesn't seem to show up as easily. Yes, let's mark this as verified and deal with the other crash in bug 1568163.

Comment 16

[Alexandru Trif, Desktop QA [:atrif]]

Thank you James!
As per comment 14 and comment 15 this issue is verified fixed. Removing the qe+ flag accordingly.