529192 - crash [@ nsXULListCellAccessible::GetAttributesInternal(nsIPersistentProperties*)]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: trivial fix

GetTable can return without a valid table, we missed one spot where we check this.

E.g. http://crash-stats.mozilla.com/report/index/06a1f27c-ce4c-4436-8a3c-960bb2091110

Comment 1

[alexander :surkov (:asurkov)]

Comment on attachment 412751 [details] [diff] [review]
trivial fix


>   // "table-cell-index" attribute
>   nsCOMPtr<nsIAccessibleTable> table;
>   GetTable(getter_AddRefs(table));
>+  if (!table)
>+    return NS_OK;

failure and warn, r=me with this

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 412751 [details] [diff] [review]
trivial fix

r=me, with the same nit as Alex, obviously :)

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #1)
> (From update of attachment 412751 [details] [diff] [review])
> 
> >   // "table-cell-index" attribute
> >   nsCOMPtr<nsIAccessibleTable> table;
> >   GetTable(getter_AddRefs(table));
> >+  if (!table)
> >+    return NS_OK;
> 
> failure and warn, r=me with this

Is there a reason we expect GetTable to always succeed?  Note we don't fail and warn in 3 other places in this same file ;)

Comment 4

[alexander :surkov (:asurkov)]

Yes, because it's a problem when XUL list cell accessible hasn't XUL listbox accessible. If it's wrong markup then we shouldn't create cell accessible in this case. If it's invalidation problem then we should address it. I think we should warn into console to know we have problems and when it happens.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

OK so we expect GetTable to always succeed inside nsXULListCellAccessible? So shall I add fails in the other 3 places?

I'm not a huge fan of warnings since the console is already so noisy/unusable.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> OK so we expect GetTable to always succeed inside nsXULListCellAccessible? So
> shall I add fails in the other 3 places?

right

> I'm not a huge fan of warnings since the console is already so noisy/unusable.

and that means we have lot of work and it shouldn't mean we should hide problems :)

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> (In reply to comment #5)
> and that means we have lot of work and it shouldn't mean we should hide
> problems :)

I agree on one level, but I won't notice the warnings. I'll add them.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Our accessibility code is probably the closest thing to a run-time XUL validator Mozilla has :)

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Our accessibility code is probably the closest thing to a run-time XUL
> validator Mozilla has :)

Haha :) I hope we'll fix this in the future.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Silly me, we already have bug 516047 with the 192 fix.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

landed on central: http://hg.mozilla.org/mozilla-central/rev/07c8d3fc2702

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 412751 [details] [diff] [review]
trivial fix

This bug recently surfaced in Thunderbird 3.1b2pre, see:
http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=nsXULListCellAccessible%3A%3AGetAttributesInternal(nsIPersistentProperties*)&version=Thunderbird%3A3.1b2pre
Specific report:
http://crash-stats.mozilla.com/report/index/311ba50e-bee1-4d7d-b50d-855fe2100413
Requesting to land this on 1.9.2.5 ASAP. Has been baking on trunk and been known to fix the problem there. Trivial fix.

Comment 13

[Mark Banner (:standard8)]

On Marco's recommendation, as this can apparently break some screen readers, I've checked this into Thunderbird's relbranch on mozilla-1.9.2 for Thunderbird 3.1 RC 1:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/96d514d88fb1

We'll look for the formal 1.9.2 landing later.

Comment 14

[Marco Zehe (:MarcoZ)]

(In reply to comment #13)
> On Marco's recommendation, as this can apparently break some screen readers,
> I've checked this into Thunderbird's relbranch on mozilla-1.9.2 for Thunderbird
> 3.1 RC 1:

Very cool, thanks! I'll let the user who reported this issue with their screen reader to test RC1 and report to me if they still experience problems. I expect not, though.

Comment 15

[Marco Zehe (:MarcoZ)]

(In reply to comment #14)
> (In reply to comment #13)
> > On Marco's recommendation, as this can apparently break some screen readers,
> > I've checked this into Thunderbird's relbranch on mozilla-1.9.2 for Thunderbird
> > 3.1 RC 1:
> 
> Very cool, thanks! I'll let the user who reported this issue with their screen
> reader to test RC1 and report to me if they still experience problems. I expect
> not, though.

As expected, the user could confirm that this problem is fixed for her in Thunderbird 3.1RC1. Thanks guys for taking this patch on your relbranch!

Comment 16

[LegNeato]

Comment on attachment 412751 [details] [diff] [review]
trivial fix

a=LegNeato for 1.9.2.6. Please land this on mozilla-1.9.2 default.

Code freeze is on 2010-06-25.

Comment 17

[Marco Zehe (:MarcoZ)]

Landed on 1.9.2 on Davidb's behalf: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4bd15fac7a13

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Marco, can you verify this is fixed in a 1.9.2 build (Firefox or Mozilla)?

Comment 19

[Marco Zehe (:MarcoZ)]

As per my comment #15, and due to the fact that I landed the patch on the default 1.9.2 branch afterwards, too, declaring this verified fixed in 1.9.2.6.