1622731 - Crash in [@ mozilla::dom::XULTreeElement::GetTreeBodyFrame]

Status: VERIFIED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

This bug is for crash report bp-01834f4a-8484-4e04-aeff-cfcab0200314.

Top 10 frames of crashing thread:

0 XUL mozilla::dom::XULTreeElement::GetTreeBodyFrame dom/xul/XULTreeElement.cpp:86
1 XUL mozilla::dom::XULTreeElement::GetColumns dom/xul/XULTreeElement.cpp:182
2 XUL nsCoreUtils::GetSensibleColumnCount accessible/base/nsCoreUtils.cpp:451
3 XUL mozilla::a11y::TableAccessible::IsProbablyLayoutTable accessible/generic/TableAccessible.cpp:136
4 XUL -[mozTablePartAccessible isLayoutTablePart] accessible/mac/mozTableAccessible.mm:17
5 XUL -[mozTablePartAccessible role] accessible/mac/mozTableAccessible.mm:37
6 XUL -[mozAccessible accessibilityIsIgnored] accessible/mac/mozAccessible.mm:136
7 AppKit __NSAccessibilityEntryPointIsAccessibilityElement_block_invoke.llvm.15474548225997367914 
8 AppKit NSAccessibilityPerformEntryPointBOOL.llvm.15474548225997367914 
9 AppKit NSAccessibilityEntryPointIsAccessibilityElement 

macOS-specific accessibility crash. It looks like a NULL-pointer dereference and seems to have started in buildid 20200305212712. I could not find non-nightly crashes but these might just be under a different signature.

Comment 1

[Marco Zehe (:MarcoZ)]

Hm, this fails in a call to GetUncomposedDoc(). This is weird in itself, since that only compares a few const members AFAICS. A few notes:

1. This element we're crashing in is probably a XULTreeGridAccessible. This is the only one that ever calls nsAccUtils::GetSensibleColumnCount. It does this from a query whether this is a layout table or not, coming from the TableAccessible base class nsXULTreeGridAccessible inherits from.
2. A XULTreeGridAccessible is most likely never going to be a layout table. The easiest way to fix this would be to override that method in XULTreeGridAccessible to always return false. Although that may wall-paper over some deeper problem.

Since there are a few Mac specific problems with tables, putting this on Morgan's and Eitan's plate for future investigation.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1622731 - In TableAccessible::IsProbablyLayoutTable, check if accessible is defunct. r?Jamie

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1622731 - Make nsCoreUtils tree/column functions null-safe. r?Jamie

Depends on D67672

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1622731 - Notify of destruction after detaching accessible wrap/proxy. r?morgan

Depends on D67673

Comment 5

[Eitan Isaacson [:eeejay]]

Looks like we are calling that on a shut down accessible. It seems that when we dispatch a destroyed notification on an element, appkit or VoiceOver query the element further (in this case they call accessibilityIsIgnored). Since our internal accessible is already shut down, things don't go well. This is a regression from bug 1618364.

STR:
With VoiceOver on, open the bookmarks window then close it.

I submittted 3 patches. Each one remedies this, but I think they are just generally good ideas.

1. Some null checks on tree/column utility functions.
2. Check if the accessible is defunct in IsProbablyLayoutTable.
3. Notify of destruction only after we detach the gecko accessible from the accessible element.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8333655213c3
Notify of destruction after detaching accessible wrap/proxy. r=morgan

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8333655213c3

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24568b324c27
In TableAccessible::IsProbablyLayoutTable, check if accessible is defunct. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/6aa798ae8615
Make nsCoreUtils tree/column functions null-safe. r=Jamie

Comment 9

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/24568b324c27
https://hg.mozilla.org/mozilla-central/rev/6aa798ae8615

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:eeejay, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 9134760 [details]
Bug 1622731 - Notify of destruction after detaching accessible wrap/proxy. r?morgan

Beta/Release Uplift Approval Request

• User impact if declined: Reproducible crash
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: With VoiceOver on, open bookmarks manager and then close it.

Firefox should not crash

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a small change that will avoid a crash for VoiceOver users.
• String changes made/needed:

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9134760 [details]
Bug 1622731 - Notify of destruction after detaching accessible wrap/proxy. r?morgan

mac a11y crash fix, approved for 75.0b10

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b1f089670404
https://hg.mozilla.org/releases/mozilla-beta/rev/766693aa2c89
https://hg.mozilla.org/releases/mozilla-beta/rev/059d2921d419

Comment 14

[Simona Badau, Desktop QA]

Reproduced the initial crash on Firefox 75 beta 9 and on Nightly 76.0a1 (Build ID: 20200320095353) on Mac OS X 10.15.
Verified as fixed using Firefox 75 beta 10 and the latest Nightly 76.0a1 on Mac OS X 10.15 (no crash occurs when following the steps from Comment 5).