404380 - Crash [@ FireDelayedAccessibleEvent ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

An inexplicable crash occurs at FireDelayedAccessibleEvent:

http://crash-stats.mozilla.com/?do_query=1&query_search=signature&query_type=contains&query=FireDelayedAccessibleEvent&date=&range_value=2&range_unit=months

It's crashing here, right after a null check on |accessibleEvent| -- which would seem to mean the event queue has invalid pointers in there. That's odd, since there are ref counted:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsDocAccessible.cpp&rev=1.199&mark=1515#1515

Most of the crashes occur when a page is loading and the doc is setting the caret to the top. We could avoid firing that event (might be helpful for perf anyway), but it doesn't really fix the root problem.

Looking for advice on this one.

Comment 1

[Ginn Chen]

no idea,

I've no idea why we need the null check of bug 333025.

Comment 2

[Aaron Leventhal]

Ginn, the null check hasn't fixed it completely, that's what's odd.

Comment 3

[Aaron Leventhal]

Attachment: Partial fix. Needs testing to ensure it doesn't break Orca

I'm concerned that it might affect Orca's knowledge of where the user is when the page loads.

Comment 4

[Aaron Leventhal]

Scott Haeger tried this and found no regressions with Orca. That would be the one screen reader I'm concerned with, since it relies on caret browsing and knowing where the Firefox caret is at all times.

Comment 5

[Ginn Chen]

Comment on attachment 289523 [details] [diff] [review]
Partial fix. Needs testing to ensure it doesn't break Orca

I found something that might be a cause of this bug.

If you add a static counter in nsCaretAccessible::AddDocSelectionListener and nsCaretAccessible::RemoveDocSelectionListener.
You will found nsCaretAccessible::AddDocSelectionListener is called more than nsCaretAccessible::RemoveDocSelectionListener.

In general case, PresShell will clear everything for us.
I don't know whether there's chance nsCaretAccessible::NotifySelectionChanged is called before the clean up.
Then mRootAccessible is a dangled pointer.

We do nsCaretAccessible::Shutdown in nsRootAccessible::Shutdown, but we don't clear mRootAccessible of nsCaretAccessible.
Could it cause a crash like the stack here?

Then why nsCaretAccessible::RemoveDocSelectionListener is not called?
The stack is like
=>[1] nsAccessNode::GetDocAccessibleFor(aPresShell = 0x9398fb8), line 666 in "nsAccessNode.cpp"
  [2] nsAccessNode::GetDocAccessibleFor(aContainer = 0x96dfca0, aCanCreate = 0), line 678 in "nsAccessNode.cpp"
  [3] nsAccessNode::GetRootAccessible(this = 0x944fec8), line 375 in "nsAccessNode.cpp"
  [4] nsDocAccessible::RemoveEventListeners(this = 0x944fec8), line 722 in "nsDocAccessible.cpp"
  [5] nsRootAccessible::RemoveEventListeners(this = 0x944fec8), line 354 in "nsRootAccessible.cpp"
  [6] nsDocAccessible::Shutdown(this = 0x944fec8), line 566 in "nsDocAccessible.cpp"
  [7] nsRootAccessible::Shutdown(this = 0x944fec8), line 951 in "nsRootAccessible.cpp"
  [8] nsDocAccessible::Destroy(this = 0x944fec8), line 548 in "nsDocAccessible.cpp"
  [9] nsRootAccessible::HandleEventWithTarget(this = 0x944fec8, aEvent = 0x981479c, aTargetNode = 0x94e3a94), line 620 in "nsRootAccessible.cpp"

1) In frame 8, we've already done gGlobalDocAccessibleCache.Remove() before calling nsRootAccessible::Shutdown(), so gGlobalDocAccessibleCache.Get() fails in frame 1.
Then nsAccessNode::GetRootAccessible() fails, and then frame 4 fails to get caretAccessible.

2) In frame 5, we do mCaretAccessible->Shutdown() before we call nsDocAccessible::RemoveEventListeners()
so even if nsDocAccessible::RemoveEventListeners can get rootAccessible, it still can't get caretAccessible.

Comment 6

[Aaron Leventhal]

Attachment: 1) Make sure we add/remove selection listener equal number of times, 2) Caret is responsible for shutting itself down with no more doc selection listeners

Good catch Ginn.

Comment 7

[Ginn Chen]

I think the counting of mDocSelectionListeners is not necessary.
If we didn't remove all the doc selection listeners, in nsRootAccessible::RemoveEventListeners, we still clears mCaretAccessible.
Therefore, there's no change to get remaining listeners removed gracefully, and we do hold a dangling pointer of mRootAccessible.

I found if I press Ctrl+T to open a new tab, a new nsDocAccessible is created.
Press Ctrl+W, I don't see a destruction of nsDocAccessible or nsDocAccessible::RemoveEventListeners.
So add/remove selection listener are not called equally.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 289523 [details] [diff] [review]
Partial fix. Needs testing to ensure it doesn't break Orca

a=beltzner on behalf of drivers

Comment 9

[Aaron Leventhal]

Ginn, if you have time please go ahead and propose a patch. Right now it's Thanksgiving time here but I should have a chance to review it.

Comment 10

[Ginn Chen]

Attachment: patch with no mDocSelectionListeners

same patch with no mDocSelectionListeners counting

Comment 11

[timeless]

I can't tell what happened to the approvals in this bug :(