1193786 - crash in mozilla::dom::Element::FindAttrValueIn(int, nsIAtom*, nsIAtom* const* const*, nsCaseTreatment)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This bug was filed from the Socorro interface and is 
report bp-c3a8e29c-af52-4c4e-b0e1-ec7e22150812.
=============================================================

This was sent to me by Birkir Gunnarsson, an accessibility friend from Deque Systems. He's experiencing this with JAWS 16, Firefox 39 on Windows 7. Alex, any ideas what could be happening here? a11y is in the stack at position 2 or 3 and down from there.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> Created attachment 8646994 [details] [diff] [review]
> patch

I guess that works around the crash, but the crash doesn't seem to make a lot of sense, and I'd expect you can still trigger it by calling Role() from other places.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> I guess that works around the crash, but the crash doesn't seem to make a
> lot of sense, and I'd expect you can still trigger it by calling Role() from
> other places.

right, there's a chance of crash. there's bunch of ways to fix that, I would prefer the one where GetCellAt() won't traverse the kids all over.

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > I guess that works around the crash, but the crash doesn't seem to make a
> > lot of sense, and I'd expect you can still trigger it by calling Role() from
> > other places.
> 
> right, there's a chance of crash. there's bunch of ways to fix that, I would
> prefer the one where GetCellAt() won't traverse the kids all over.

I have no idea what you are trying to say.

Comment 5

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

What is a better solution here?

Comment 6

[alexander :surkov (:asurkov)]

the proposed solution I believe is good, it makes stuff faster and fixes the crash. As a follow up I would make GetCellAt() to operate on cells array, since it makes Role computation much faster (this would also fixed the crash). As another option, we probably shouldn't create HTMLTableHeaderCellAccessible for ARIA grids, we need a test case for that though.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> the proposed solution I believe is good, it makes stuff faster and fixes the

Ok, I miseed that the calls where looping, now this patch makes more sense.

> crash. As a follow up I would make GetCellAt() to operate on cells array,

what do you mean cells array?

> since it makes Role computation much faster (this would also fixed the
> crash). As another option, we probably shouldn't create
> HTMLTableHeaderCellAccessible for ARIA grids, we need a test case for that
> though.

it also seems kind of wierd that getting the cell next to the current one returns the same one or different ones in a loop, but maybe there's a reasonable case where that happens.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> > crash. As a follow up I would make GetCellAt() to operate on cells array,
> 
> what do you mean cells array?

either cache cells for each row like we do embedded characters in hypertext or make sure on accessible tree creation that a row doesn't contain other accessibles than cells. I think I like 2nd more.


> > since it makes Role computation much faster (this would also fixed the
> > crash). As another option, we probably shouldn't create
> > HTMLTableHeaderCellAccessible for ARIA grids, we need a test case for that
> > though.
> 
> it also seems kind of wierd that getting the cell next to the current one
> returns the same one or different ones in a loop, 

I think it happens because GetCellAt() runs through all kids, so when you ask GetCellAt(1) for a cell at 0 index then you will iterate through the zero index cell.

Comment 9

[Marco Zehe (:MarcoZ)]

Comment on attachment 8646994 [details] [diff] [review]
patch

r=me.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08caf7dd0502

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08caf7dd0502