1194859 - crash in mozilla::a11y::ARIAGridCellAccessible::GroupPosition()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-199b1006-4e4d-4751-bbfc-dcb2a2150813.
=============================================================

This signature spiked in 41b1 (because this code is new to 41).

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::a11y::ARIAGridCellAccessible::GroupPosition() 	accessible/generic/ARIAGridAccessible.cpp
1 	xul.dll 	mozilla::a11y::Accessible::NativeAttributes() 	accessible/generic/Accessible.cpp
2 	xul.dll 	mozilla::a11y::HyperTextAccessible::NativeAttributes() 	accessible/generic/HyperTextAccessible.cpp
3 	xul.dll 	mozilla::a11y::ARIAGridCellAccessible::NativeAttributes() 	accessible/generic/ARIAGridAccessible.cpp
4 	xul.dll 	mozilla::a11y::Accessible::Attributes() 	accessible/generic/Accessible.cpp
5 	xul.dll 	mozilla::a11y::ia2Accessible::get_attributes(wchar_t**) 	accessible/windows/ia2/ia2Accessible.cpp

Comment 1

[(Away)]

[Tracking Requested - why for this release]: topcrash on 41

Comment 2

[(Away)]

http://hg.mozilla.org/releases/mozilla-beta/annotate/be2423a7ec20/accessible/generic/ARIAGridAccessible.cpp#l693

Something in this chain is null: Table()->AsAccessible()->GetContent().
I think it is the |Table()|.

Comment 3

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 41 and 42 as crashes are bad.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 5

[Marco Zehe (:MarcoZ)]

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

r=me. Do we have other places in our table code where we have similar constructs that could bite us in the future?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #5)
> Comment on attachment 8649278 [details] [diff] [review]
> patch
> 
> r=me. Do we have other places in our table code where we have similar
> constructs that could bite us in the future?

it seems no other places.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e89ce960e5f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e89ce960e5f

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Alexander, given that this was a top crash for 41, do you want to request uplift to Beta on this patch? It looks safe to me but you are the best judge. Thanks!

Comment 10

[alexander :surkov (:asurkov)]

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

Approval Request Comment
[Feature/regressing bug #]: bug 1177268
[User impact if declined]: assistive technologies users keep crashing
[Describe test coverage new/current, TreeHerder]: not covered
[Risks and why]: no risk, a null check
[String/UUID change made/needed]: no

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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

Fix a top crash, taking it.

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/b28e5271cb63

Comment 13

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6dda342701ab

Comment 14

[(Away)]

This crash is still seen on all channels where the patch landed. Looking at the disassembly of beta4, I'm pretty certain the null pointer is |Table()|, not |table|.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 16

[Marco Zehe (:MarcoZ)]

Comment on attachment 8653627 [details] [diff] [review]
patch2

Review of attachment 8653627 [details] [diff] [review]:
-----------------------------------------------------------------

Yikes! *That* is where it failed! OK, let's hope that we've now kicked it into obedience! Thanks! r=me.

Comment 17

[Marco Zehe (:MarcoZ)]

Comment on attachment 8653627 [details] [diff] [review]
patch2

Umm ... I said: r=me!

Comment 18

[(Away)]

Comment on attachment 8653627 [details] [diff] [review]
patch2

Review of attachment 8653627 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/ARIAGridAccessible.cpp
@@ +690,5 @@
>  ARIAGridCellAccessible::GroupPosition()
>  {
>    int32_t count = 0, index = 0;
> +  Accessible* table = Table();
> +  if (table && nsCoreUtils::GetUIntAttr(table->->AsAccessible()->GetContent(),

There's a double arrow here.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb84e5f9c664

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb84e5f9c664

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 22

[Marco Zehe (:MarcoZ)]

Comment on attachment 8653627 [details] [diff] [review]
patch2

Approval Request Comment
[Feature/regressing bug #]: bug 1177268
[User impact if declined]: Crashes.
[Describe test coverage new/current, TreeHerder]: On Central, follow-up/corrected patch to previous one from this bug.
[Risks and why]: Null check.
[String/UUID change made/needed]: None.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8653627 [details] [diff] [review]
patch2

> [Risks and why]: Null check.
I don't see a risk evaluation here?! Please be more explicit for future uplift requests.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1098c02a9ca

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ae10d6a0419a