Crash in mozilla::a11y::Accessible::RemoveChild

RESOLVED FIXED in Firefox -esr52

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: calixte, Assigned: surkov)

Tracking

(Blocks 2 bugs, {crash, regression})

55 Branch
mozilla55
Unspecified
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5255+ fixed, firefox53 wontfix, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [clouseau], crash signature)

Attachments

(3 attachments)

Reporter

Description

2 years ago
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
Flags: needinfo?(surkov.alexander)
Assignee

Updated

2 years ago
Duplicate of this bug: 1360122
Assignee

Comment 2

2 years ago
(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
Flags: needinfo?(surkov.alexander)
Assignee

Comment 3

2 years ago
(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
Assignee

Comment 4

2 years ago
Posted patch patchSplinter Review
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
Assignee: nobody → surkov.alexander
Attachment #8865929 - Flags: review?(dbolter)
Attachment #8865929 - Flags: review?(dbolter) → review+
Assignee

Updated

2 years ago
Keywords: leave-open
Assignee

Comment 7

2 years ago
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
Assignee

Comment 8

2 years ago
(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
Assignee

Comment 10

2 years ago
Posted patch stop bleedingSplinter Review
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.
Attachment #8867242 - Flags: review?(dbolter)
Attachment #8867242 - Flags: review?(dbolter) → review+

Updated

2 years ago
Crash Signature: [@ mozilla::a11y::Accessible::RemoveChild] → [@ mozilla::a11y::Accessible::RemoveChild] [@ mozilla::a11y::DocAccessible::ContentRemoved]
Assignee

Updated

2 years ago
Blocks: 1270916
Looks fixed, nice work.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Assignee

Comment 14

2 years ago
(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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I assume we want this on Beta54 and ESR52 too (along with bug 1342433 re-landed)?
Flags: needinfo?(surkov.alexander)
Assignee

Comment 20

2 years ago
(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
Flags: needinfo?(surkov.alexander)
Assignee

Comment 21

2 years ago
Posted patch backportSplinter Review
Attachment #8870986 - Flags: review?(dbolter)
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.
Attachment #8870986 - Flags: review?(dbolter) → review+
Assignee

Comment 23

2 years ago
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
Attachment #8870986 - Flags: approval-mozilla-aurora?
Comment on attachment 8870986 [details] [diff] [review]
backport

Fx54 is on Beta now.
Attachment #8870986 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8870986 [details] [diff] [review]
backport

Fix a crash. Beta54+. Should be in 54 beta 11.
Attachment #8870986 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
Were you going to request ESR52 backport on this too?
Flags: needinfo?(surkov.alexander)
Too late now.
Assignee

Comment 30

2 years ago
(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
Flags: needinfo?(surkov.alexander)
Please request approval still. Worst case, we can maybe get it into 52.3 still.
Flags: needinfo?(surkov.alexander)
Assignee

Comment 32

2 years ago
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.
Flags: needinfo?(surkov.alexander)
Attachment #8870986 - Flags: approval-mozilla-esr52?
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.
Assignee

Comment 34

2 years ago
(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
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')
Flags: needinfo?(surkov.alexander)
Assignee

Comment 36

2 years ago
(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.
Flags: needinfo?(surkov.alexander)
Comment on attachment 8870986 [details] [diff] [review]
backport

No regressions from this change, let's uplift to ESR52.3
Attachment #8870986 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.