Crash in [@ mozilla::a11y::TextRange::CommonParent]
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
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)
(deleted),
text/x-phabricator-request
|
Details |
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
- Go to Phabricator's Differencial page
- Long tap on a word in a line in the diff
- Drag left accessible caret to extend the range to start of the line
- Keep dragging over the line number columun
Then, I see this crash.
Reporter | ||
Updated•2 years ago
|
Comment 1•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 2•2 years ago
|
||
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
.
Updated•2 years ago
|
Comment 3•2 years ago
|
||
(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.
Assignee | ||
Comment 4•2 years ago
|
||
(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.
Assignee | ||
Comment 5•2 years ago
|
||
:masayuki, can you still reproduce this with the STR in comment 0?
Reporter | ||
Comment 6•2 years ago
|
||
Yeah, still reproducible in GV: 105.0a1-20220809093338.
Assignee | ||
Comment 7•2 years ago
|
||
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.
Updated•2 years ago
|
Updated•2 years ago
|
Comment 8•2 years ago
|
||
(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).
Assignee | ||
Comment 9•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 10•2 years ago
|
||
Comment 11•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Description
•