Closed Bug 1213281 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Disability Access APIs, defect)

44 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox47 --- affected
firefox-esr45 --- affected
b2g-v2.5 --- fixed

People

(Reporter: davidb, Assigned: surkov)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

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
Note that you need a Facebook account to be able to follow these steps.
Crash Signature: [@ mozilla::a11y::DocAccessible::UpdateTreeOnInsertion(mozilla::a11y::Accessible*)] → [@ mozilla::a11y::DocAccessible::UpdateTreeOnInsertion(mozilla::a11y::Accessible*)] [@ mozilla::a11y::DocAccessible::UpdateTreeOnInsertion]
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
Attached patch patchSplinter Review
Attachment #8691545 - Flags: review?(dbolter)
Assignee: nobody → surkov.alexander
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?
Attachment #8691545 - Flags: review?(dbolter) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2dbc87221d6005db2074a0e4782b8d4198e0ae
Bug 1213281 - crash in mozilla::a11y::DocAccessible::UpdateTreeOnInsertion, r=davidb
https://hg.mozilla.org/mozilla-central/rev/cf2dbc87221d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.
Flags: needinfo?(surkov.alexander)
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.
Attachment #8691545 - Flags: approval-mozilla-aurora?
David, could you please verify this crash has gone away on the latest Nightly? Thanks!
Flags: needinfo?(dbolter)
Comment on attachment 8691545 [details] [diff] [review]
patch

Crash fixes are good. Let's uplift to Aurora44.
Attachment #8691545 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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)
Flags: needinfo?(dbolter)
This doesn't appear to apply cleanly to aurora, can we get a rebased patch for it?
(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?
Flags: needinfo?(surkov.alexander)
(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
Flags: needinfo?(surkov.alexander)
Attached patch crashSplinter Review
Flags: needinfo?(surkov.alexander)
Attachment #8695422 - Flags: review?(dbolter)
Comment on attachment 8695422 [details] [diff] [review]
crash

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

r=me, looks familiar ;)
Attachment #8695422 - Flags: review?(dbolter) → review+
This is verified based on comment 15.
Status: RESOLVED → VERIFIED
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
You need to log in before you can comment on or make changes to this bug.