1363027 - 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-d1134016-3717-44c1-a4c2-6acf70170508.
=============================================================

There are 35 crashes in nightly 55 with buildid 20170506030204. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1362063.

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

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Calixte Denizet (:calixte) from comment #0)
> This bug was filed from the Socorro interface and is 
> report bp-d1134016-3717-44c1-a4c2-6acf70170508.
> =============================================================
> 
> There are 35 crashes in nightly 55 with buildid 20170506030204. In analyzing
> the backtrace, the regression may have been introduced by patch [1] to fix
> bug 1362063.

this is a new signature of bug 1360122

Comment 3

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #2)

> > There are 35 crashes in nightly 55 with buildid 20170506030204. In analyzing
> > the backtrace, the regression may have been introduced by patch [1] to fix
> > bug 1362063.
> 
> this is a new signature of bug 1360122

for the record: which was regressed by bug 1353094

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

add debugging assert for crashes like [1], which asserts at
MOZ_DIAGNOSTIC_ASSERT(aChild->mParent, "No parent");
in Accessible::RemoveChild [2]

[1] https://crash-stats.mozilla.com/report/index/59d03edf-2e1e-4a99-8c8d-652670170509
[2] https://hg.mozilla.org/mozilla-central/annotate/1fda52a1f3b8/accessible/generic/Accessible.cpp#l2136

Comment 5

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a473a4e0eed937aea85cd97725dd93a85fbdf0c
Bug 1363027 - diagnostic asserts for Accessible::RemoveChild, r=davidb

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a473a4e0eed

Comment 7

[alexander :surkov (:asurkov)]

so there's a diagnostic crash of deparented child [1], which is deparated during event coalescence [2], which is likely a regression from bug 1270916

[1] https://crash-stats.mozilla.com/report/index/72e44e03-d4c7-4edf-9602-9f4060170511
[2] https://hg.mozilla.org/mozilla-central/annotate/ce2218406119/accessible/generic/DocAccessible.cpp#l1987

Comment 8

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #7)
> so there's a diagnostic crash of deparented child [1], which is deparated
> during event coalescence [2], which is likely a regression from bug 1270916

oh right, an accessible may die during the event coalescence [1]

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/base/NotificationController.cpp?q=NotificationController%3A%3ADropMutationEvent&redirect_type=single#266

Comment 9

[alexander :surkov (:asurkov)]

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

Comment 10

[alexander :surkov (:asurkov)]

Attachment: stop bleeding

I'm not sure I fully understand the new event coalescence system, it seems overcomplicated, so here's stop-bleeding patch, that should fix a crash. I will file a follow up bug for event coalescence problem.

Comment 11

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26f5d5167d2c502932b251dbedd3341914d1b589
Bug 1363027 - keep removing accessible alive during event coalescence, r=davidb

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/26f5d5167d2c

Comment 13

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

Looks fixed, nice work.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #13)
> Looks fixed, nice work.

right, keep fingers crossed, no more new crashes so far. If that's a case, then I will do a backport of it, hopefully it will help with whole bunch of other crashes we have on beta.

Comment 15

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

Unfortunately I learned we didn't build nightlies for a couple of days... so this might have been a premature closing. Let's reopen and watch.

Comment 16

[alexander :surkov (:asurkov)]

We may call it fixed, no new crashes after May 15:
https://crash-stats.mozilla.com/signature/?product=Firefox&version=55.0a1&signature=mozilla%3A%3Aa11y%3A%3ADocAccessible%3A%3AContentRemoved&date=%3E%3D2017-05-17T12%3A50%3A26.000Z&date=%3C2017-05-24T12%3A50%3A26.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-build_id&_sort=-date&page=1

Comment 17

[Julien Cristau [:jcristau]]

(In reply to alexander :surkov from comment #16)
> We may call it fixed, no new crashes after May 15:
> https://crash-stats.mozilla.com/signature/?product=Firefox&version=55.
> 0a1&signature=mozilla%3A%3Aa11y%3A%3ADocAccessible%3A%3AContentRemoved&date=%
> 3E%3D2017-05-17T12%3A50%3A26.000Z&date=%3C2017-05-24T12%3A50%3A26.
> 000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_colum
> ns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-
> build_id&_sort=-date&page=1

Thanks, resolving.

Comment 18

[alexander :surkov (:asurkov)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I assume we want this on Beta54 and ESR52 too (along with bug 1342433 re-landed)?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> I assume we want this on Beta54 and ESR52 too 

yes, the work on a backport is in progress, nightly and beta code bases are different and the patch is not applied cleanly

> (along with bug 1342433
> re-landed)?

might be not necessary, let's go with this bug for the start

Comment 21

[alexander :surkov (:asurkov)]

Attachment: backport

Comment 22

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

Comment on attachment 8870986 [details] [diff] [review]
backport

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

I guess there is some risk here since this area of code changed.

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 8870986 [details] [diff] [review]
backport

Approval Request Comment
[Feature/Bug causing the regression]:bug 1270916
[User impact if declined]: crashes, sec-issues like bug 1318645
[Is this code covered by automated tests?]:a fair coverage, not this crash particularly though
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]:reasonable risk
[Why is the change risky/not risky?]:a somewhat simple patch for a generally complicated code
[String changes made/needed]:no

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8870986 [details] [diff] [review]
backport

Fx54 is on Beta now.

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8870986 [details] [diff] [review]
backport

Fix a crash. Beta54+. Should be in 54 beta 11.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/38c9c1ad194a

Comment 27

[Andrei Vaida [:avaida]]

(In reply to alexander :surkov from comment #23)
> [Is this code covered by automated tests?]:a fair coverage, not this crash
> particularly though
> [Has the fix been verified in Nightly?]:yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Alexander's assessment on manual testing needs.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Were you going to request ESR52 backport on this too?

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Too late now.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> Were you going to request ESR52 backport on this too?

(In reply to Al Billings [:abillings] from comment #29)
> Too late now.

sorry somehow slept off my radar, it would have been nice of course to have it there

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Please request approval still. Worst case, we can maybe get it into 52.3 still.

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 8870986 [details] [diff] [review]
backport

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: this is a fix for number of sec bugs and crashes like bug 1318645 and bug 1309686
Fix Landed on Version: 55/54
Risk to taking this patch (and alternatives if risky): not risky, couple of kungfu-death-grips to keep an object alive
String or UUID changes made by this patch:no 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 33

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

Looking at the crash volume, I am not convinced this needs to be fixed for ESR52 at all. Wontfix for ESR52.2. Let's give this another cycle, if the volume remains low, perhaps this can ride the ESR59 train.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Ritu Kothari (:ritu) from comment #33)
> Looking at the crash volume, I am not convinced this needs to be fixed for
> ESR52 at all. Wontfix for ESR52.2. Let's give this another cycle, if the
> volume remains low, perhaps this can ride the ESR59 train.

just curious, what number of crashes is not low volume crash? mozilla::a11y::Accessible::Elm signature [1] in 52.1.2esr is about 40 a week, I didn't check others, they might have same ratio.

[1] https://crash-stats.mozilla.com/signature/?version=52.1.2esr&signature=mozilla%3A%3Aa11y%3A%3AAccessible%3A%3AElm&date=%3E%3D2017-05-31T20%3A17%3A00.000Z&date=%3C2017-06-07T20%3A17%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Doesn't apply :
grafting 397884:5a473a4e0eed "Bug 1363027 - diagnostic asserts for Accessible::RemoveChild, r=davidb"
merging accessible/generic/DocAccessible.cpp
 warning: conflicts while merging accessible/generic/DocAccessible.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 36

[alexander :surkov (:asurkov)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #35)
> Doesn't apply :
> grafting 397884:5a473a4e0eed "Bug 1363027 - diagnostic asserts for
> Accessible::RemoveChild, r=davidb"
> merging accessible/generic/DocAccessible.cpp
>  warning: conflicts while merging accessible/generic/DocAccessible.cpp!
> (edit, then use 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

all you need is the backport path, the diagnostic assert patch shouldn't be backported.

Comment 37

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

Comment on attachment 8870986 [details] [diff] [review]
backport

No regressions from this change, let's uplift to ESR52.3

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c5eaa2d51b9f