Closed Bug 1193786 Opened 5 years ago Closed 5 years ago

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

Categories

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

39 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: MarcoZ, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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.
Flags: needinfo?(surkov.alexander)
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Flags: needinfo?(surkov.alexander)
Attachment #8646994 - Flags: review?(mzehe)
(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.
(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.
(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.
What is a better solution here?
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.
(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.
(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 on attachment 8646994 [details] [diff] [review]
patch

r=me.
Attachment #8646994 - Flags: review?(mzehe) → review+
https://hg.mozilla.org/mozilla-central/rev/08caf7dd0502
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.