1838718 - Crash in [@ -[mozTableCellAccessible moxRowHeaderUIElements]]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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  

Comment 1

[James Teh [:Jamie]]

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.

Comment 2

[James Teh [:Jamie]]

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.

Comment 3

[Donal Meehan [:dmeehan]]

: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?

Comment 4

[James Teh [:Jamie]]

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.

Comment 5

[James Teh [:Jamie]]

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[James Teh [:Jamie]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[James Teh [:Jamie]]

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?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[James Teh [:Jamie]]

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.

Comment 14

[James Teh [:Jamie]]

The crash in ESR115 should be fixed by bug 1850643 once it gets uplifted. That's not the correct fix, but that patch is probably needed to fix other (more rare) potential instances of this anyway.