Closed Bug 1531346 Opened 1 year ago Closed 1 year ago

Crash in [@ gfxSkipCharsIterator::SetOriginalOffset]

Categories

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

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- fixed
firefox65 --- wontfix
firefox66 + fixed
firefox67 --- fixed

People

(Reporter: marcia, Assigned: eeejay)

Details

(Keywords: crash, regression, regressionwindow-wanted)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-d89cd5ef-0e6d-4f46-bfa6-7a3cf0190210.

Seen while looking at nightly crash stats, but present on other branches as well: https://bit.ly/2VofEpX. Lots of comments mention crashing when working on some kind of banking or reconciliation. Not sure if this is a signature change, but I could not find an existing bug to cover this crash.

All of the crashes are various flavors of Windows, and the crash reason is EXCEPTION_STACK_OVERFLOW.

Comments:

  • keeps crashing when reconciling payments and deposits
  • The BMW website always does this, hope their cars are more reliable!
  • f it is NOT possible to click on ab line item in the Bank Reconciliation in QuickBooks Online, PLEASE DISABLE the ability to do so
  • EVERY time I click on a check that has not cleared in the "Reconcile" process of our checking account, this error occurs. Please fix this as its very disruptive and I need to be able to click on items in the Register Reconcile process!!!!
Top 10 frames of crashing thread:

0 xul.dll gfxSkipCharsIterator::SetOriginalOffset gfx/thebes/gfxSkipChars.cpp:68
1 xul.dll nsTextFrame::EnsureTextRun layout/generic/nsTextFrame.cpp:2903
2 xul.dll nsTextFrame::GetRenderedText layout/generic/nsTextFrame.cpp:9738
3 xul.dll nsTextEquivUtils::AppendTextEquivFromTextContent accessible/base/nsTextEquivUtils.cpp:127
4 xul.dll static nsresult nsTextEquivUtils::AppendFromAccessible accessible/base/nsTextEquivUtils.cpp:173
5 xul.dll nsTextEquivUtils::AppendFromAccessibleChildren accessible/base/nsTextEquivUtils.cpp:162
6 xul.dll static nsresult nsTextEquivUtils::AppendFromAccessible accessible/base/nsTextEquivUtils.cpp:198
7 xul.dll nsTextEquivUtils::AppendFromAccessibleChildren accessible/base/nsTextEquivUtils.cpp:162
8 xul.dll static void nsTextEquivUtils::GetTextEquivFromSubtree accessible/base/nsTextEquivUtils.h:73
9 xul.dll static nsresult nsTextEquivUtils::AppendFromValue accessible/base/nsTextEquivUtils.cpp

This bug is for crash report bp-d89cd5ef-0e6d-4f46-bfa6-7a3cf0190210.

The stack in this report looks like we're running into infinite recursion in the a11y code: there's a 4-frame recursive cycle in accessible/base/nsTextEquivUtils that repeats (indefinitely?) from frame 7 downwards.

Component: Layout: Text and Fonts → Disability Access APIs

I just came across another related signature which has some similar comments about crashing when using quickbooks, and has some of the same stacks as noted in Comment 1.

Crash Signature: [@ gfxSkipCharsIterator::SetOriginalOffset] → [@ gfxSkipCharsIterator::SetOriginalOffset] [@ _chkstk | nsTextFrame::EnsureTextRun]

Marco, can you take a look at this or help find an owner to investigate?

Flags: needinfo?(mzehe)

Passing the NI onto our team lead, since I am currently heads-down in a project.

Flags: needinfo?(mzehe) → needinfo?(jteh)

Bouncing the NI to Eitan, since he expressed interest in looking into this.

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

So this is definitely due to cyclical ancestor-descendant relationship. A comment on crash report gave me a pretty solid STR:

  1. Go to https://www.joefresh.com/us/Categories/Women/Women%27s-New-Arrivals/c/10008?q=%3Arelevance&sort=relevance&q=:relevance&sort=relevance&sid=homepage|Promo%20Tiles:%203x|3|WK11%20Promo%20Tiles:%20Mar%207
  2. With a screen reader on tab to the "Sort by" combo box. That should be enough to cause the crash. You can also do this with a11y devtools simply by selecting the combobox in the tree.

The cause of the cyclical relationship here is because the aria-activedescendent that's set on the div[role=combobox] points to an ancestor element. When Accessible::Value is called we end up infinitely recursing. What is needed in this case in a cycle check in Accessible::CurrentItem.

I traced that problem widget to a react-select library (http://jedwatson.github.io/react-select/). I suspect the examples there don't crash because the combobox role is applied to an input element which has a different Value() codepath.

I suspect a large proportion of those crashes will be remedied by the followup patch, but some might still slip through the cracks because there are other ways to create cyclical relationships via aria-owns even if they are highly obscure.

Flags: needinfo?(eitan)
Assignee: nobody → eitan
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/546397372dd7
Check for cyclical relationship with aria-activedescendant. r=MarcoZ
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Is this something you think is upliftable and low risk for 66 release? I can still get it into the RC2 build.

Flags: needinfo?(eitan)

Looking at the patch I think it would be fine, and it would be nice to ship this fix in 66.

Yeah. We should uplift this if possible

Flags: needinfo?(eitan)

Comment on attachment 9049403 [details]
Bug 1531346 - Check for cyclical relationship with aria-activedescendant. r?MarcoZ!

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: Crashes on certain websites
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • 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 simply cycle check
  • String changes made/needed:
Attachment #9049403 - Flags: approval-mozilla-beta?

Comment on attachment 9049403 [details]
Bug 1531346 - Check for cyclical relationship with aria-activedescendant. r?MarcoZ!

Crash fix, affects a11y, simple fix.
OK for uplift to m-r for the RC2 build.

Attachment #9049403 - Flags: approval-mozilla-release+
Attachment #9049403 - Flags: approval-mozilla-beta?
Attachment #9049403 - Flags: approval-mozilla-beta-

The gfxSkipCharsIterator::SetOriginalOffset signature has over 200 hits on ESR60 in the last week. Can we nominate this for ESR60 approval too? It grafts cleanly.

Flags: needinfo?(eitan)

Comment on attachment 9049403 [details]
Bug 1531346 - Check for cyclical relationship with aria-activedescendant. r?MarcoZ!

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: High crash volume on ESR.
  • User impact if declined: High crash volume
  • Fix Landed on Version: 66
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The fix grafts cleanly and has been tested for a while in all other channels.
  • String or UUID changes made by this patch:
Flags: needinfo?(eitan)
Attachment #9049403 - Flags: approval-mozilla-esr60?

Comment on attachment 9049403 [details]
Bug 1531346 - Check for cyclical relationship with aria-activedescendant. r?MarcoZ!

Crash fix, often seems to affect banking. Crash volume nearly 0 after the fix landed in 66/67. Let's take it for 60.7esr.

Attachment #9049403 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.