1822170 - [Fx View] Tabbing from Tab Pickup header with VoiceOver on causing browser to freeze up

Status: VERIFIED FIXED

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

Description

[Kelly Cochrane [:kcochrane]]

I'm not sure when this was introduced, but it's occurring in latest central and Nightly builds. It is not occurring on the latest release build (111).

STR:

• Open the browser to Firefox View
• The window needs to be wide enough to where the “Firefox View” text is visible on the left-hand side of the Fx View page
• VoiceOver needs to be on
• Tab to focus from the entire main content area, down to the Tab Pickup header, then down to anything right after the Tab Pickup header (different based on if you’re synced/signed in), but it doesn’t seem to matter what comes after the header.
• Notice the browser then freezes up indefinitely

Comment 1

[Sam Foster [:sfoster] (he/him)]

I'm not able to reproduce this on beta/111, but I can reproduce on nightly/112. Its a pretty bad hang, it just beach-balls with no sign of unhanging - I have to force quit Nightly from the activity monitor.
For this feature and this platform, anyone using Voiceover is going to have a bad time, so I'm setting it as S2.

Comment 2

[Sam Foster [:sfoster] (he/him)]

mozregression worked ok - I just had to force-quit the bad builds. It gave me:

Last good revision: 7ce58b7344140d588da812ab25170590c9a00144
First bad revision: 34f8b14723e0531cb05a9350c477ab431f30323c
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7ce58b7344140d588da812ab25170590c9a00144&tochange=34f8b14723e0531cb05a9350c477ab431f30323c

:eeejay, that push log looks like you might have some insight into this. Is this is a newly revealed fxview bug, or a regression from one of your patches. It seems to be moving focus between the <summary> and the <details> content that triggers the hang when using voiceover. But its possible also this is just the first place we ran into the issue and the same thing reproduces elsewhere.

Comment 3

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

Setting Regressed by field after analyzing regression range found by mozregression in comment #2.

Comment 4

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

Set release status flags based on info from the regressing bug 1730095

Comment 5

[Eitan Isaacson [:eeejay]]

Thanks for this report. I can reproduce and I'm looking for a fix.

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1822170 - Handle any kind of NSRanges provided to AXAttributedStringForRange. r?morgan!

This patch remedies 3 things:

1. Fix conversion from NSRange to GeckoTextMarkerRange. We were adding the start offset twice. Oops.
2. Clamp given range to actual text size. Since we are messing with the offset fields directly we need to do our own checks here.
3. Don't allow an infinite loop in CachedTextMarkerRange::AttributedText

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3ea9faf7c95
Handle any kind of NSRanges provided to AXAttributedStringForRange. r=Jamie

Comment 8

[Iulian Moraru]

Backed out for causing multiple browser-a11y failures.

• Push with failures

• Failure log for when it fails on browser_text_leaf.js

• Failure log for when it fails on browser_attributed_text.js

• Failure log for when it fails with assertion

• Failure log for when it fails with a different assertion

• Backout link

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b67ef7bd374
Handle any kind of NSRanges provided to AXAttributedStringForRange. r=Jamie

Comment 10

[Marian-Vasile Laza]

Backed out changeset 1b67ef7bd374 (Bug 1822170) for assertion failures on Accessible.cpp.
Backout link
Push with failures <--> ba
Failure Log

Comment 11

[Eitan Isaacson [:eeejay]]

Resubmitted tweaked patch for review.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30b7d9c22176
Handle any kind of NSRanges provided to AXAttributedStringForRange. r=Jamie

Comment 13

[Cristian Tuns]

Backed out for causing mochitest failures on CachedTextMarker.mm

• Backout link
• Push with failures
• Failure Log
• Failure line: Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Cannot proceed further in attribute run), at /builds/worker/checkouts/gecko/accessible/mac/CachedTextMarker.mm:331

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcd8c992b7f5
Handle any kind of NSRanges provided to AXAttributedStringForRange. r=Jamie

Comment 15

[Marian-Vasile Laza]

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

Comment 16

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

The patch landed in nightly and beta is affected.
:eeejay, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 17

[Eitan Isaacson [:eeejay]]

Comment on attachment 9323048 [details]
Bug 1822170 - Handle any kind of NSRanges provided to AXAttributedStringForRange. r?morgan!

Beta/Release Uplift Approval Request

• User impact if declined: If users enable the a11y cache independently, or if we have an experiment cohort before 113 for caching in MacOS they will be affected by this hang.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Original STR in bug
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This bug fix has test coverage for both caching and non-caching configurations.
• String changes made/needed:
• Is Android affected?: No

Comment 18

[Hani Yacoub, Desktop QA]

Managed to reproduce the freeze on Firefox Nightly 112.0a1(2023-03-14) but couldn't reproduce it on Firefox Beta.
The bug is verified as fixed on Firefox Nightly 113.0a1 (2023-03-24).

Comment 19

[Dianna Smith [:diannaS]]

Comment on attachment 9323048 [details]
Bug 1822170 - Handle any kind of NSRanges provided to AXAttributedStringForRange. r?morgan!

Approved for 112.0b7

Comment 20

[Dianna Smith [:diannaS]]

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

Comment 21

[Hani Yacoub, Desktop QA]

The bug isn't reproducible on macOS 11.6 on Firefox Beta 112.0b9 and neither on older Firefox Beta versions as mentioned in comment 18.