1213281 - crash in mozilla::a11y::DocAccessible::UpdateTreeOnInsertion(mozilla::a11y::Accessible*)

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-107671ed-2b58-4875-92ba-12a2a2151008.
=============================================================

Some STR from crash report comments:
1) Go to messenger.com
2) Type in the name of a contact
3) Click that contact
4) Wait
5) Crash

and,

messenger.com contact search (left sidebar), typing a search then clicking a contact from the results (or simply hitting TAB) crashes the browser 

More reports here: http://is.gd/sAf15p

Comment 1

[Marco Zehe (:MarcoZ)]

Note that you need a Facebook account to be able to follow these steps.

Comment 2

[Brian Birtles (:birtles)]

I'm seeing this repeatedly on Aurora. Using the less restrictive signature it seems there have been 113 crashes in the last week at the time of writing:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3ADocAccessible%3A%3AUpdateTreeOnInsertion

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40f59b488271

Comment 5

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae6eb66055bb

Comment 6

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

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

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

::: accessible/generic/DocAccessible.cpp
@@ +2160,5 @@
>  void
>  DocAccessible::PutChildrenBack(nsTArray<RefPtr<Accessible> >* aChildren,
>                                 uint32_t aStartIdx)
>  {
> +  nsTArray<nsRefPtr<Accessible> > containers;

Better as "nsTArray<RefPtr<Accessible>" ;)

@@ +2197,5 @@
>  
>    // And put it back where it belongs to.
>    aChildren->RemoveElementsAt(aStartIdx, aChildren->Length() - aStartIdx);
>    for (uint32_t idx = 0; idx < containers.Length(); idx++) {
> +    if (containers[idx]->IsInDocument()) {

OK I guess? When does this fail?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #6)

> > +  nsTArray<nsRefPtr<Accessible> > containers;
> 
> Better as "nsTArray<RefPtr<Accessible>" ;)

yeah, try build says same

> @@ +2197,5 @@
> >  
> >    // And put it back where it belongs to.
> >    aChildren->RemoveElementsAt(aStartIdx, aChildren->Length() - aStartIdx);
> >    for (uint32_t idx = 0; idx < containers.Length(); idx++) {
> > +    if (containers[idx]->IsInDocument()) {
> 
> OK I guess? When does this fail?

I don't see a code path but it looks like that something destroys container accessibles before we process them. I'll add an assertion for this case.

Comment 8

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17d257c4d305

Comment 9

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2dbc87221d6005db2074a0e4782b8d4198e0ae
Bug 1213281 - crash in mozilla::a11y::DocAccessible::UpdateTreeOnInsertion, r=davidb

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Brian Birtles (:birtles)]

Alexander, do you mind requesting approval to land on Aurora?

Without this affected users are unable to use facebook messenger without crashing so I hope we don't ship to release.

Comment 12

[Marco Zehe (:MarcoZ)]

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

Approval Request Comment
[Feature/regressing bug #]: bug 1133213 or follow-ups (aria-owns work)
[User impact if declined]: Crash on messenger.com and other pages with anything that might use accessibility APIs. Also see comments above.
[Describe test coverage new/current, TreeHerder]: On m-c for days.
[Risks and why]: None known.
[String/UUID change made/needed]: None.

Comment 13

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

David, could you please verify this crash has gone away on the latest Nightly? Thanks!

Comment 14

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

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

Crash fixes are good. Let's uplift to Aurora44.

Comment 15

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

(In reply to Ritu Kothari (:ritu) from comment #13)
> David, could you please verify this crash has gone away on the latest
> Nightly? Thanks!

Yes, I agree with Ritu's comment 14. (No crashes for 45)

Comment 16

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

This doesn't appear to apply cleanly to aurora, can we get a rebased patch for it?

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Wes Kocher (:KWierso) from comment #16)
> This doesn't appear to apply cleanly to aurora, can we get a rebased patch
> for it?

it may have a different implementation, so it might require a new patch. The search doesn't give me any crash reports, not sure why https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=mozilla%3A%3Aa11y%3A%3ADocAccessible%3A%3AUpdateTreeOnInsertion%28mozilla%3A%3Aa11y%3A%3AAccessible*%29#tab-reports. Do we have a stack for aurora?

Comment 18

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

(In reply to alexander :surkov from comment #17)
> Do we have a stack for aurora?

Yes, try http://is.gd/KWN3kC

Example: https://crash-stats.mozilla.com/report/index/2944e72d-531d-4535-b553-ae2852151203

Comment 19

[alexander :surkov (:asurkov)]

Attachment: crash

Comment 20

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

Comment on attachment 8695422 [details] [diff] [review]
crash

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

r=me, looks familiar ;)

Comment 21

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

This is verified based on comment 15.

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d61e62d4fc42

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'mozilla::a11y::DocAccessible::UpdateTreeOnInsertion':
 - nightly(version 50):0 crashes from 2016-06-06.
 - aurora (version 49):0 crashes from 2016-06-07.
 - beta   (version 48):0 crashes from 2016-06-06.
 - release(version 47):54 crashes from 2016-05-31.
 - esr    (version 45):9 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       0       0       0       0       0
 - aurora        0       0       0       0       0       0       0
 - beta          0       0       0       0       0       0       0
 - release       2       9       8       5       9       9      11
 - esr           1       1       1       0       2       0       3

Affected platform: Windows