Closed Bug 1838718 Opened 1 year ago Closed 1 year ago

Crash in [@ -[mozTableCellAccessible moxRowHeaderUIElements]]

Categories

(Core :: Disability Access APIs, defect)

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
116 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox-esr115 118+ fix-optional
firefox114 --- wontfix
firefox115 --- wontfix
firefox116 --- fixed

People

(Reporter: aryx, Assigned: Jamie)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

macOS crash which started with Firefox v113 (90 crash reports). There is less crash volume for v114 (15 reports so far).

Crash report: https://crash-stats.mozilla.org/report/index/be6d2987-da04-46b9-96f1-b1c9b0230615

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0  XUL  -[mozTableCellAccessible moxRowHeaderUIElements]  accessible/mac/mozTableAccessible.mm:482
1  XUL  -[MOXAccessibleBase accessibilityAttributeValue:]  accessible/mac/MOXAccessibleBase.mm:142
2  AppKit  NSAccessibilityGetObjectForAttributeUsingLegacyAPI  
3  AppKit  ___NSAccessibilityEntryPointValueForAttribute_block_invoke.748  
4  AppKit  NSAccessibilityPerformEntryPointObject  
5  AppKit  _NSAccessibilityEntryPointValueForAttribute  
6  AppKit  -[NSObject accessibilityArrayAttributeCount:]  
7  AppKit  -[NSObject _accessibilityArrayAttributeCount:clientError:]  
8  AppKit  GetAttributeValueCount  
9  HIServices  _AXXMIGGetAttributeValueCount  
Flags: needinfo?(jteh)

I think this could only happen if IsTableCell() returns true but AsTableCellBase() returns null. That shouldn't really possible, but I vaguely recall there might have been an edge case where it was. That should be fixed by bug 1832261 and bug 1832228.

Depends on: 1832228, 1832261
Flags: needinfo?(jteh)

If this spikes in 114, we could land a simple null check on 115, as those other bugs are probably a bit risky to uplift.

Severity: -- → S3

:jamie this is too late for Fx114.
We are in the final week of Beta for Fx115, do you plan on doing anything this week that is safe to uplift?

Flags: needinfo?(jteh)

I hadn't planned on it unless the crash spiked, given that we should already have a fix in 116. The crash rate looks pretty low. However, a beta fix is pretty trivial, so I can look into that. I don't want to land that on central though, since the correct (but uplift-risky) fix has already happened elsewhere.

Looking at this more closely, I'm less certain this is a trivial fix. There are probably other methods that will suffer in the same way. Given the low crash rate, the lack of certainty here and the time available, I think we're better off letting this slide to 116.

Closing as fixed by bug 1832261 and bug 1832228.

Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(jteh)
Resolution: --- → FIXED

Any concerns about this on ESR115? Note that we can wait and see for a cycle or two first if needed.

Assignee: nobody → jteh
Flags: needinfo?(jteh)
Target Milestone: --- → 116 Branch

For this and other reasons (see bug 1840200), I think we're going to need to eventually uplift bug 1832261 and bug 1832228 to ESR115, as well as a few other patches. Unfortunately, timing is not on our side here and these patches are pretty large, which makes this a little risky right now. I'm hoping I can convince release management :) to take these uplifts despite their size once they've baked a bit without issues in Nightly/beta/maybe release.

Flags: needinfo?(jteh)

I can't set flags for the 117 cycle yet, but we can revisit then after 116 gets to release.

Hi Jamie, just checking in to see what your thoughts are for ESR115 at this point.

Flags: needinfo?(jteh)

Bug 1832261 and bug 1832228 (plus the regression bug 1838540) had some time on nightly, a full cycle in beta and 2 weeks on release without any reported problems. So, I'm pretty confident they're fine for users, but they're still pretty large patches. We'll also need these if we're eventually going to uplift some hit testing fixes; e.g. reinstating bug 1828373 and bug 1825611.

How much does patch size figure in the ESR uplift risk assessment?

Flags: needinfo?(jteh)

I think the size mostly comes into play when deciding how long we feel it needs to bake first. If the patches have shipped to most of our users and we have a good handle on what issues came up with them, that would bode well in my opinion.

Per Slack discussion, we're going to work out the uplifts next cycle since RC week is next week.

We'll also need bug 1835967. :(
So in all, to deal with this table crash among other table issues, we'll need bug 1835967, bug 1832261, bug 1832228 and bug 1838540. The first three apply cleanly. The last one will need a slight conflict massage in a test file.

You need to log in before you can comment on or make changes to this bug.