1346518 - Crash in mozilla::a11y::Accessible::RemoveChild

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-23d9a615-5bf1-4994-b600-66af02161130.
=============================================================

There 4 crashes on nightly 55 with buildid 20170310110248. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1321384.

[1] https://hg.mozilla.org/mozilla-central/rev?node=dea9f3038c4d1cad37ac5d9908dbaef42fabecf2

Comment 1

[alexander :surkov (:asurkov)]

How is it different from bug 1321384?

Comment 2

[alexander :surkov (:asurkov)]

Did you mean Accessible::RemoveChild signature and crashes like this:
https://crash-stats.mozilla.com/report/index/eaab4989-c625-4174-904f-431cf2170310

Comment 3

[Calixte Denizet (:calixte)]

Oh my bad... I made a mistake, I picked up the wrong crash report. Yes it's exactly this signature.

Comment 4

[alexander :surkov (:asurkov)]

We have a bug 1342433 that should resolve the issue leading to a stack from comment #2.

Also I don't see how the tree may get corrupted by a tree recreation under a wrong container, the latter code should fix all issue introduced by that. So this stack may be unrelated with aria-hidden issue.

Comment 5

[alexander :surkov (:asurkov)]

there's one more bug 1347667 for this signature, let's if these two bugs (comment #4) fix this bug.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #5)
> there's one more bug 1347667 for this signature, let's if these two bugs
> (comment #4) fix this bug.

there's a stack not covered by those two https://crash-stats.mozilla.com/report/index/cd485e66-7442-40a5-9ea2-9e3712170320

Comment 7

[alexander :surkov (:asurkov)]

Attachment: assertions

1) add empty parent assertion, in other words split ASSERT(aChild->mParent == this) into two
2) get unwanted wording change back from https://hg.mozilla.org/mozilla-central/diff/dea9f3038c4d/accessible/generic/Accessible.cpp

Comment 8

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0046bf50e45ff3986c6192a428a8a275f6641ba9
Bug 1346518 - extend Accessible::RemoveChild debugging assertions, r=yzen

Comment 9

[alexander :surkov (:asurkov)]

Attachment: assert2

add extra assertion, possibly catching cases like https://crash-stats.mozilla.com/report/index/cd485e66-7442-40a5-9ea2-9e3712170320

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/0046bf50e45f

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8849673 [details] [diff] [review]
assert2

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

looks good.

Comment 13

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0bb65bdf413cda117ec915f8aa97d262b94f642
Bug 1346518 - extend Accessible::RemoveChild debugging assertions, part2, r=yzen

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

What's the status of this bug?  it's leave-open, so the landings (and other bugs mentioned here as possible fixes) haven't moved this to RESOLVED.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Randell Jesup [:jesup] from comment #15)
> What's the status of this bug?  it's leave-open, so the landings (and other
> bugs mentioned here as possible fixes) haven't moved this to RESOLVED.

this crash signature may be caused by number of reasons, few of them them were addressed, at least one is not yet fixed, for example, this one https://crash-stats.mozilla.com/report/index/cd485e66-7442-40a5-9ea2-9e3712170320

Comment 17

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

Alex are the asserts helping?

Here is a query that adds the reason as a column. You can see most of them are failing on no parent.
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3AAccessible%3A%3ARemoveChild&date=%3E%3D2017-03-20T20%3A38%3A00.000Z&date=%3C2017-03-27T20%3A38%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=moz_crash_reason&_sort=-build_id&_sort=-date&page=1

Comment 18

[alexander :surkov (:asurkov)]

I filed a bug 1351414 that could help with 'No parent' assertions

Comment 19

[Bryan Price]

I can recreate hitting this bug today by going to https://www.att.com/wireless/

https://crash-stats.mozilla.com/report/index/bp-a9c905ff-6b24-4fb0-bce0-5f06d2170329
https://crash-stats.mozilla.com/report/index/bp-afa67bc9-c265-4649-b018-2f41a2170329
https://crash-stats.mozilla.com/report/index/bp-439cf829-f641-4067-a0a8-e92212170329
https://crash-stats.mozilla.com/report/index/bp-bbf14cbf-3d8e-465f-b4de-987962170329

Rather annoying. /sniff.

Comment 20

[David Parks [:handyman]]

The issues on att.com that cause this crash should be repaired by the patches in bug 1353094.  We think there may be other causes to this crash, tho.

Comment 21

[Jim Mathies [:jimm]]

diagnostic assert:

https://hg.mozilla.org/mozilla-central/annotate/3c68d659c2b7/accessible/generic/Accessible.cpp#l2135

Comment 22

[Nicholas Nethercote [inactive]]

This signature is the #5 Window top crash in Nightly 20170421030241.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Nicholas Nethercote [:njn] from comment #22)
> This signature is the #5 Window top crash in Nightly 20170421030241.

I expect bug 1353094 helping it.

Comment 25

[alexander :surkov (:asurkov)]

Technically bug 1353094 is a blocking one, not a dupe. Also do we have data that no other cases of this crash signature has left?

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

There are still some crashes on 53, 54, and 55. If this isn't a duplicate I guess we can re-open the bug. However with so few crashes, I don't think release management needs to track this issue.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> There are still some crashes on 53, 54, and 55. If this isn't a duplicate I
> guess we can re-open the bug. However with so few crashes, I don't think
> release management needs to track this issue.

I believe that 55 crashes should be fixed by recent bug 1363027, 53 and 54 crashes like [1] can be fixed by backporting bug 1347667, should I request the backporting or we don't care enough about this low volume crash to take a risk of backporting?

https://crash-stats.mozilla.com/report/index/409c6855-6a0d-4305-b1dd-80e850170529

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Relevant patches from comment 27 have been backported to ESR52. Calling this fixed there as well.