Closed Bug 1363027 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Version:
55 Branch
Platform:
Unspecified
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 55+ fixed
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: calixte, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(3 files)

patch
1021 bytes, patch
davidb
: review+
Details | Diff | Splinter Review
stop bleeding
1.19 KB, patch
davidb
: review+
Details | Diff | Splinter Review
backport
1.26 KB, patch
davidb
: review+
gchang
: approval-mozilla-beta+
ritu
: approval-mozilla-esr52+
Details | Diff | Splinter Review 
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)
(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)
(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
Attached patch patch — — Splinter 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+
Keywords: leave-open
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
Attached patch stop bleeding — — Splinter 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+
Crash Signature: [@ mozilla::a11y::Accessible::RemoveChild] → [@ mozilla::a11y::Accessible::RemoveChild] [@ mozilla::a11y::DocAccessible::ContentRemoved]
Blocks: 1270916
Looks fixed, nice work.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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
Closed: 7 years ago → 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I assume we want this on Beta54 and ESR52 too (along with bug 1342433 re-landed)?
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Flags: needinfo?(surkov.alexander)
(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)
Attached patch backport — — Splinter 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+

    
    

    
  
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.

    
    

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

    
    

    
  
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.

          tracking-firefox-esr52:
          --- → 55+

    
    

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

    
    

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: