Closed Bug 1781169 Opened 2 years ago Closed 2 years ago

Crash in [@ mozilla::a11y::TextRange::CommonParent]

Categories

(Core :: Disability Access APIs, defect)

All
Android
defect

Tracking

()

RESOLVED FIXED
106 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- unaffected
firefox103 --- disabled
firefox104 --- disabled
firefox105 --- disabled
firefox106 --- fixed

People

(Reporter: masayuki, Assigned: Jamie)

References

Details

(Keywords: crash, regression, Whiteboard: [geckoview:m105] [geckoview:m106])

Crash Data

Attachments

(1 file)

Crash report: https://crash-stats.mozilla.org/report/index/15c632bd-95ff-4365-82e8-7391a0220726

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so mozilla::a11y::TextRange::CommonParent const accessible/base/TextRange.cpp:517
1 libxul.so mozilla::a11y::TextRange::Crop accessible/base/TextRange.cpp:194
2 libxul.so mozilla::a11y::HyperTextAccessibleBase::CroppedSelectionRanges const accessible/basetypes/HyperTextAccessibleBase.cpp:662
3 libxul.so mozilla::a11y::HyperTextAccessibleBase::SelectionBoundsAt accessible/basetypes/HyperTextAccessibleBase.cpp:686
4 libxul.so mozilla::a11y::SessionAccessibility::SendTextSelectionChangedEvent accessible/android/SessionAccessibility.cpp:415
5 libxul.so mozilla::a11y::ProxyCaretMoveEvent accessible/android/Platform.cpp:147
6 libxul.so mozilla::a11y::DocAccessibleParent::RecvCaretMoveEvent accessible/ipc/DocAccessibleParent.cpp:426
7 libxul.so {virtual override thunk} 
8 libxul.so mozilla::a11y::PDocAccessibleParent::OnMessageReceived ipc/ipdl/PDocAccessibleParent.cpp:8994
9 libxul.so mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:6616

STR

  1. Go to Phabricator's Differencial page
  2. Long tap on a word in a line in the diff
  3. Drag left accessible caret to extend the range to start of the line
  4. Keep dragging over the line number columun

Then, I see this crash.

Eitan, did any big Android a11y changes land in 102? This crash looks like a regression in Fenix 102.0a1. I see crash reports from Fenix Nightly and Beta, but not Release, even though 102 has been in Release for a full cycle.

Flags: needinfo?(eitan)
Hardware: Unspecified → All

The crash range makes sense because caching is only enabled on nightly (and was briefly enabled on beta).

Jamie, my theory is that TextRange::CommonParent is getting null when retrieving a cached GetChildAtOffset, and then CommonParent crashes because it de-references aAcc1 when trying to get a parent.

Is there a state (specifically in the context of recieving a text selection changed event) when the offset cache/hierarchy cache might be out of sync? If nothing comes to mind, I can just paper over it in TextRange::CommonParent.

Flags: needinfo?(eitan)
Flags: needinfo?(jteh)

(In reply to Eitan Isaacson [:eeejay] from comment #2)

The crash range makes sense because caching is only enabled on nightly (and was briefly enabled on beta).

In that case, I'll set the status flags for Release 103 and Beta 104 to disabled, since caching is not enabled in Release or Beta.

Depends on: 1782146

(In reply to Eitan Isaacson [:eeejay] from comment #2)

Is there a state (specifically in the context of recieving a text selection changed event) when the offset cache/hierarchy cache might be out of sync? If nothing comes to mind, I can just paper over it in TextRange::CommonParent.

Per bug 1772170, it is possible for the selection Accessibles/offsets to be out of sync for a short time. However, the checks I added here should deal with that case. So, it looks like we have a case here where the HyperText cache is out of sync with the tree.

I'm seeing other instances of that here and there too. I've filed bug 1782146 to fix a few cases I've spotted where we're not invalidating the HyperText cache where we should. Hopefully, that will fix this, but we'll have to wait and see.

Flags: needinfo?(jteh)

:masayuki, can you still reproduce this with the STR in comment 0?

Flags: needinfo?(masayuki)

Yeah, still reproducible in GV: 105.0a1-20220809093338.

Flags: needinfo?(masayuki)

The code I added to protect against out of date selection ranges checks that the offsets aren't greater than the character count. However, the offset could be 0 when there are no characters and this check wouldn't catch that. If there were no children, this could result in GetChildAtOffset returning null.

I'm not really sure how we should handle selection being in an empty container; I'm not even sure how that happens. For now, it might make sense to extend the checks above to cover this 0 character case and see if it helps.

Whiteboard: [geckoview:m105]
Whiteboard: [geckoview:m105] → [geckoview:m105] [geckoview:m106]

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

I'm not really sure how we should handle selection being in an empty container; I'm not even sure how that happens. For now, it might make sense to extend the checks above to cover this 0 character case and see if it helps.

Jamie, will you have time to make that change in Nightly 106? This crash is Fenix Nightly's #3 top crash (though the crash volume is still pretty low because this crash only affects Nightly).

Flags: needinfo?(jteh)

In bug 1772170, I added code to ensure that the cached offsets are valid.
However, this didn't account for the possibility that one of the containers contained 0 characters.
In that case, offset 0 is itself invalid.
If there were also no children, this might in GetChildAtOffset being called and returning null, causing a crash when retrieving selection.

Assignee: nobody → jteh
Status: NEW → ASSIGNED
Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c273352d2c18 Ensure that the cached start/end containers contain characters before returning them in DocAccessibleParent::SelectionRanges. r=eeejay
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: