1188415 - crash in mozilla::a11y::DocAccessibleParent::CheckDocTree()

Status: RESOLVED WORKSFORME

(Core :: Disability Access APIs, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-96e50d50-40a5-4ea6-86d3-6f3d52150727.
=============================================================

More reports here: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3ACheckDocTree%28%29

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: add document tree consistancy checks to more message handlers

another diagnostic patch.  Now that we've show some previous message screwed up the tree so RecvHide event operated on garbage lets try and narrow down which message did that by checking on entry and exit of all msg handlers that deal with tree mutation.  Hopefully we'll see crashes leaving one of these handlers which would tend to indicate that handler started with an ok tree and screwed it up.

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e20a7ea2c16

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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

Comment 4

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

I checked the last 16 stacks manually since I don't know (if I can or) how to do a clever query. They were all of the RecvHideEvent variety.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #4)
> I checked the last 16 stacks manually since I don't know (if I can or) how
> to do a clever query. They were all of the RecvHideEvent variety.

RecvHideEvent() calls CheckDocTree twice, was it the first or second time?

Comment 6

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

Example of first check in RecvHideEvent:
https://crash-stats.mozilla.com/report/index/392e2602-e553-4319-981d-b8bc62150804

Example of second check in RecvHideEvent:
https://crash-stats.mozilla.com/report/index/a8d9e72d-b3db-437c-9fa7-97aef2150804

Comment 7

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

Currently top crash on nightly.

Comment 8

[Jim Mathies [:jimm]]

#6 overall, #1 for browser.

Comment 9

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

Copying from bug 1170049
> > Trevor, maybe we should turn CheckDocTree into a boolean and return false
> > instead of crashing. Then at the callsite crash if it returns false? This
> > would make it a lot easier to see where we are crashing in socorro.
> 
> that's fine with me, but I'm kind of in the middle of fixing 1189277 for
> another day or two.

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: make CheckDocTree return if the document tree is in a sane state

Comment 11

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

Comment on attachment 8648909 [details] [diff] [review]
make CheckDocTree return if the document tree is in a sane state

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

cool. this will bitrot patch on bug 1193919 of course...

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2843215083ea

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2843215083ea

Comment 15

[Jim Mathies [:jimm]]

This is on aurora as well, can we uplift?

Comment 16

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

(In reply to Jim Mathies [:jimm] from comment #15)
> This is on aurora as well, can we uplift?

I think we should yes. Trevor? (The new macros will make sure we don't call these diagnostics on release, which is good)

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #16)
> (In reply to Jim Mathies [:jimm] from comment #15)
> > This is on aurora as well, can we uplift?
> 
> I think we should yes. Trevor? (The new macros will make sure we don't call
> these diagnostics on release, which is good)

I'm not sure I understand the point? besides I kind of thought you wanted to disable e10s + a11y on aurora all  together?

Comment 18

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

(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> (In reply to David Bolter [:davidb] from comment #16)
> > (In reply to Jim Mathies [:jimm] from comment #15)
> > > This is on aurora as well, can we uplift?
> > 
> > I think we should yes. Trevor? (The new macros will make sure we don't call
> > these diagnostics on release, which is good)
> 
> I'm not sure I understand the point? besides I kind of thought you wanted to
> disable e10s + a11y on aurora all  together?

My point was basically bug 1193919. I agree if we disable e10s + a11y there won't be a need to uplift.

Comment 19

[Jim Mathies [:jimm]]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> (In reply to David Bolter [:davidb] from comment #16)
> > (In reply to Jim Mathies [:jimm] from comment #15)
> > > This is on aurora as well, can we uplift?
> > 
> > I think we should yes. Trevor? (The new macros will make sure we don't call
> > these diagnostics on release, which is good)
> 
> I'm not sure I understand the point? besides I kind of thought you wanted to
> disable e10s + a11y on aurora all  together?

We haven't made that decision yet. In the mean time we should uplift what we can if it fixes a bad crash.

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to Jim Mathies [:jimm] from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > (In reply to David Bolter [:davidb] from comment #16)
> > > (In reply to Jim Mathies [:jimm] from comment #15)
> > > > This is on aurora as well, can we uplift?
> > > 
> > > I think we should yes. Trevor? (The new macros will make sure we don't call
> > > these diagnostics on release, which is good)
> > 
> > I'm not sure I understand the point? besides I kind of thought you wanted to
> > disable e10s + a11y on aurora all  together?
> 
> We haven't made that decision yet. In the mean time we should uplift what we
> can if it fixes a bad crash.

yeah, but we don't have a fix yet.


(In reply to David Bolter [:davidb] from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #17)
> > (In reply to David Bolter [:davidb] from comment #16)
> > > (In reply to Jim Mathies [:jimm] from comment #15)
> > > > This is on aurora as well, can we uplift?
> > > 
> > > I think we should yes. Trevor? (The new macros will make sure we don't call
> > > these diagnostics on release, which is good)
> > 
> > I'm not sure I understand the point? besides I kind of thought you wanted to
> > disable e10s + a11y on aurora all  together?
> 
> My point was basically bug 1193919. I agree if we disable e10s + a11y there
> won't be a need to uplift.

yeah, making sure CheckDocTre is a nop on release is a good reason.

Comment 21

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

(In reply to Trevor Saunders (:tbsaunde) from comment #20)

> yeah, making sure CheckDocTre is a nop on release is a good reason.

Trevor can you request uplift?

Comment 22

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

Comment on attachment 8648909 [details] [diff] [review]
make CheckDocTree return if the document tree is in a sane state

Useful diagnostic information, approved for uplift to aurora.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ec7d19880f9

Comment 24

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

Trevor do we still need the CheckDocTree diagnostic? (I'm fine if you want to keep it until we ship e10s a11y)

In the meantime I think we can probably close this bug WORKSFORME?

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #24)
> Trevor do we still need the CheckDocTree diagnostic? (I'm fine if you want
> to keep it until we ship e10s a11y)

Well, its nightly only so I think we might as well keep it.

> In the meantime I think we can probably close this bug WORKSFORME?

if we aren't seeing any crashes sure, other wise I guess we should investigate those, though I'm not really sure how we would.`

Comment 26

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

Right I'm not seeing any reports anymore. Let's reopen if we do.

Comment 27

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.