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

(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

(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

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

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

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

(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

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.

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

Looks fixed, nice work.

(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.

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

(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.

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

(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 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 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 on attachment 8870986 [details] [diff] [review] backport Fx54 is on Beta now.

Comment on attachment 8870986 [details] [diff] [review] backport Fix a crash. Beta54+. Should be in 54 beta 11.

(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.

Were you going to request ESR52 backport on this too?

Too late now.

(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

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

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.

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.

(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')

(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 on attachment 8870986 [details] [diff] [review] backport No regressions from this change, let's uplift to ESR52.3