Closed Bug 1188415 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
Unspecified
Windows NT
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
e10s + ---
firefox42 --- affected
firefox43 --- affected

People

(Reporter: davidb, Assigned: tbsaunde)

References

Details

(Keywords: crash, topcrash-win)

Crash Data

Attachments

(2 files)

add document tree consistancy checks to more message handlers
2.87 KB, patch
davidb
: review+
Details | Diff | Splinter Review
make CheckDocTree return if the document tree is in a sane state
4.93 KB, patch
davidb
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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
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.
Attachment #8640047 - Flags: review?(dbolter)
Attachment #8640047 - Flags: review?(dbolter) → review+
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.
(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?
Assignee: nobody → tbsaunde+mozbugs
Currently top crash on nightly.
#6 overall, #1 for browser.
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 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...
Attachment #8648909 - Flags: review?(dbolter) → review+
This is on aurora as well, can we uplift?
(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)
Flags: needinfo?(tbsaunde+mozbugs)
(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?
Flags: needinfo?(tbsaunde+mozbugs)
(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.
(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.
(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.
Assignee: tbsaunde+mozbugs → jmathies
(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?
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(tbsaunde+mozbugs)
Attachment #8648909 - Flags: approval-mozilla-aurora?
Assignee: jmathies → nobody
Assignee: nobody → tbsaunde+mozbugs
Blocks: e10sa11y2
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.
Attachment #8648909 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Crash Signature: [@ mozilla::a11y::DocAccessibleParent::CheckDocTree()] → [@ mozilla::a11y::DocAccessibleParent::CheckDocTree()] [@ mozilla::a11y::DocAccessibleParent::CheckDocTree]
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?
Flags: needinfo?(tbsaunde+mozbugs)
(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.`
Flags: needinfo?(tbsaunde+mozbugs)
Right I'm not seeing any reports anymore. Let's reopen if we do.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Depends on: 1197181
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: