1681072 - Slack: Opening the Keyboard Shortcuts listing in the side bar causes Firefox to freeze if VoiceOver is active.

Status: RESOLVED FIXED

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

Description

[Marco Zehe (:MarcoZ)]

[Tracking Requested - why for this release]: Hang that can cause the browser to be unresponsive as a whole. Should be fixed for Firefox 85, either directly or as an uplift from 86 during the 85 beta cycle.

Steps:

1. Turn on VoiceOver and open a tab to the Mozilla Slack workspace.
2. Tab to the Help menu, choose Keyboard Shortcuts.
3. Start navigating in the side bar of keyboard shortcuts.

Result: Either immediately, or after a short while, Firefox starts to freeze and spin up the CPU fan like crazy. VoiceOver goes "Firefox busy"...

From now on, if the tab is reopened with the keyboard shortcut listing open in the side bar, the tab will freeze the whole browser immediately if VoiceOver is active.

Comment 1

[Julien Cristau [:jcristau]]

Is this a regression?

Comment 2

[Marco Zehe (:MarcoZ)]

Unknown at this time, if this is a regression from bug 1675301 or another expression of a similar problem. But Firefox 85 is going to be our customer preview for Mac VoiceOver support, and if possible, we should not freeze on something as this.

Comment 3

[Morgan Reschenberg [:morgan]]

Looks like this is an issue with some text API:

frame #8: 0x0000000101f0a954 XUL`mozilla::a11y::PDocAccessiblePlatformExtParent::SendTextForRange(this=0x000000013108f5e0, aID=<unavailable>, aStartOffset=0x00007ffeefbfb5d8, aEndContainer=<unavailable>, aEndOffset=<unavailable>, aText=0x00007ffeefbfb500) at PDocAccessiblePlatformExtParent.cpp:358:20 [opt]
    frame #9: 0x0000000106a441df XUL`mozilla::a11y::GeckoTextMarkerRange::Text(this=<unavailable>) const at GeckoTextMarker.mm:394:47 [opt]
    frame #10: 0x0000000106a4a7a0 XUL`-[MOXTextMarkerDelegate moxStringForTextMarkerRange:](self=<unavailable>, _cmd=<unavailable>, textMarkerRange=<unavailable>) at MOXTextMarkerDelegate.mm:113:16 [opt]
    frame #11: 0x0000000106a4a7de XUL`-[MOXTextMarkerDelegate moxLengthForTextMarkerRange:](self=<unavailable>, _cmd=<unavailable>, textMarkerRange=<unavailable>) at MOXTextMarkerDelegate.mm:117:13 [opt]
    frame #12: 0x0000000106a47477 XUL`-[MOXAccessibleBase accessibilityAttributeValue:forParameter:](self=0x000000011ccacc00, _cmd=<unavailable>, attribute="AXLengthForTextMarkerRange", parameter=0x000000012687e240) at MOXAccessibleBase.mm:357:17 [opt]

The top of the stack was all messaging stuff, I'll post the whole thing in another comment for context.

Comment 4

[Morgan Reschenberg [:morgan]]

 frame #0: 0x00007fff6dd99882 libsystem_kernel.dylib`__psynch_cvwait + 10
    frame #1: 0x00007fff6de5a457 libsystem_pthread.dylib`_pthread_cond_wait + 748
    frame #2: 0x00000001001779a4 libmozglue.dylib`mozilla::detail::ConditionVariableImpl::wait_for(this=0x000000013354d6c0, lock=<unavailable>, a_rel_time=0x00007ffeefbfb258) at ConditionVariable_posix.cpp:146:7 [opt]
    frame #3: 0x000000010150ccb8 XUL`mozilla::OffTheBooksCondVar::Wait(this=0x000000013354d6a0, aDuration=(mValue = 250000000)) at BlockingResourceBase.cpp:529:20 [opt]
    frame #4: 0x0000000101d67357 XUL`mozilla::ipc::MessageChannel::WaitForSyncNotify(bool) [inlined] mozilla::Monitor::Wait(this=<unavailable>, aDuration=<unavailable>) at Monitor.h:36:59 [opt]
    frame #5: 0x0000000101d6734e XUL`mozilla::ipc::MessageChannel::WaitForSyncNotify(this=0x0000000132ff9110, (null)=<unavailable>) at MessageChannel.cpp:2342 [opt]
    frame #6: 0x0000000101d665c4 XUL`mozilla::ipc::MessageChannel::Send(this=0x0000000132ff9110, aMsg=<unavailable>, aReply=0x00007ffeefbfb430) at MessageChannel.cpp:1517:27 [opt]
    frame #7: 0x0000000101d73bab XUL`mozilla::ipc::IProtocol::ChannelSend(this=<unavailable>, aMsg=0x000000012683b400, aReply=<unavailable>) at ProtocolUtils.cpp:519:29 [opt]
    frame #8: 0x0000000101f0a954 XUL`mozilla::a11y::PDocAccessiblePlatformExtParent::SendTextForRange(this=0x000000013108f5e0, aID=<unavailable>, aStartOffset=0x00007ffeefbfb5d8, aEndContainer=<unavailable>, aEndOffset=<unavailable>, aText=0x00007ffeefbfb500) at PDocAccessiblePlatformExtParent.cpp:358:20 [opt]
    frame #9: 0x0000000106a441df XUL`mozilla::a11y::GeckoTextMarkerRange::Text(this=<unavailable>) const at GeckoTextMarker.mm:394:47 [opt]
    frame #10: 0x0000000106a4a7a0 XUL`-[MOXTextMarkerDelegate moxStringForTextMarkerRange:](self=<unavailable>, _cmd=<unavailable>, textMarkerRange=<unavailable>) at MOXTextMarkerDelegate.mm:113:16 [opt]
    frame #11: 0x0000000106a4a7de XUL`-[MOXTextMarkerDelegate moxLengthForTextMarkerRange:](self=<unavailable>, _cmd=<unavailable>, textMarkerRange=<unavailable>) at MOXTextMarkerDelegate.mm:117:13 [opt]
    frame #12: 0x0000000106a47477 XUL`-[MOXAccessibleBase accessibilityAttributeValue:forParameter:](self=0x000000011ccacc00, _cmd=<unavailable>, attribute="AXLengthForTextMarkerRange", parameter=0x000000012687e240) at MOXAccessibleBase.mm:357:17 [opt]
    frame #13: 0x00007fff3149461c AppKit`___NSAccessibilityEntryPointValueForAttributeWithParameter_block_invoke.821 + 557
    frame #14: 0x00007fff3148fa74 AppKit`NSAccessibilityPerformEntryPointObject + 16
    frame #15: 0x00007fff31490ed1 AppKit`NSAccessibilityEntryPointValueForAttributeWithParameter + 172
    frame #16: 0x00007fff31298931 AppKit`CopyParameterizedAttributeValue + 460
    frame #17: 0x00007fff31e8af6f HIServices`_AXXMIGCopyParameterizedAttributeValue + 409
    frame #18: 0x00007fff31ead0bf HIServices`_XCopyParameterizedAttributeValue + 608
    frame #19: 0x00007fff31e6af64 HIServices`mshMIGPerform + 174
    frame #20: 0x00007fff33bb8304 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 41
    frame #21: 0x00007fff33bb8250 CoreFoundation`__CFRunLoopDoSource1 + 541
    frame #22: 0x00007fff33bb6d79 CoreFoundation`__CFRunLoopRun + 2270
    frame #23: 0x00007fff33bb5e3e CoreFoundation`CFRunLoopRunSpecific + 462
    frame #24: 0x00007fff327e2abd HIToolbox`RunCurrentEventLoopInMode + 292
    frame #25: 0x00007fff327e27d5 HIToolbox`ReceiveNextEventCommon + 584
    frame #26: 0x00007fff327e2579 HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 64
    frame #27: 0x00007fff30e28039 AppKit`_DPSNextEvent + 883
    frame #28: 0x00007fff30e26880 AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1352
    frame #29: 0x000000010566364f XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x0000000100653920, _cmd=<unavailable>, mask=18446744073709551615, expiration=0x00007fff8b465618, mode="kCFRunLoopDefaultMode", flag=YES) at nsAppShell.mm:171:24 [opt]
    frame #30: 0x00007fff30e1858e AppKit`-[NSApplication run] + 658

Comment 5

[Morgan Reschenberg [:morgan]]

Ah okay the slack code has a class that looks like:

.c-icon--cmd-small::before {
    content: "\E24B";
}

which corresponds to the "icons" they're using for the command symbol, shift symbol, etc.
That content property contains a character that is in the unicode "private use" area: https://codepoints.net/U+E24B?lang=en
so it doesn't have anything "defined" about it, which means (I think) that VO will struggle to handle it and other chars in that range unless we specifically tell it how. trying to come up with a test case

Comment 6

[Morgan Reschenberg [:morgan]]

Looks like this is due to infinite recursion in NormalizeForward, having a hard time getting a stack trace, but will keep trying. Here's the manual (inverted) stack trace I was doing with printf debugging:

HyperTextAccessibleWrap::TextForRange
HyperTextIterator::Next
HyperTextIterator::NormalizeForward
recurses infinitely in normalize forward

Comment 7

[Eitan Isaacson [:eeejay]]

I think this may be a core accessibility issue - the order of the children in #shortcut-1-0 is not right.. It should be

• listitem
  ** text leaf ("Move focus through messages")
  ** graphic ("up arrow key")
  ** text leaf ("or ")
  ** graphic ("down arrow key")

but instead the "or " text leaf is the last child. I think this is conflicting with the text offsets and putting us in a circular loop.

Note - if you save the whole html page to disk and load it, the order is correct. I think this is some xhr triggered dom mutation in the actual slack app..

Once we figure out the core fix, we should put some fail-safes in the hypertext wrapper code to not allow infinite recursion.

Comment 8

[Julien Cristau [:jcristau]]

(In reply to Marco Zehe (:MarcoZ) from comment #2)

Unknown at this time, if this is a regression from bug 1675301 or another expression of a similar problem. But Firefox 85 is going to be our customer preview for Mac VoiceOver support, and if possible, we should not freeze on something as this.

85 is now on beta. If I am to track this for that release can we get an assignee on the bug?

Comment 9

[Marco Zehe (:MarcoZ)]

Eitan said yesterday that he has some leads, so assigning to him for now.

Comment 10

[Julien Cristau [:jcristau]]

Thanks Marco and Eitan!

Comment 11

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1681072 - Don't recurse into link if it is in more than one offset.

This is a safeguard for endless recursion in HyperTextIterator::NormalizeForward. Will catch similar corruptions found in bug 1682692.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/348cd8e482df
Don't recurse into link if it is in more than one offset. r=MarcoZ

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/348cd8e482df

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 9193567 [details]
Bug 1681072 - Don't recurse into link if it is in more than one offset.

Beta/Release Uplift Approval Request

• User impact if declined: Hangs or crashes with certain element mutations and their text representations on MacOS for VoiceOver.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: bug 1682692
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Safeguards against infinite recursion.
• String changes made/needed: None.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9193567 [details]
Bug 1681072 - Don't recurse into link if it is in more than one offset.

Approved for 85.0b4.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0200b1a8d025