Closed
Bug 404380
Opened 17 years ago
Closed 17 years ago
Crash [@ FireDelayedAccessibleEvent ]
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: aaronlev)
References
Details
(Keywords: access, crash, Whiteboard: Endgame drivers: we want the last patch as well -- it's more of a core fix)
Crash Data
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
ginnchen+exoracle
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aaronlev
:
review+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
no idea,
I've no idea why we need the null check of bug 333025.
Assignee | ||
Comment 2•17 years ago
|
||
Ginn, the null check hasn't fixed it completely, that's what's odd.
Assignee | ||
Comment 3•17 years ago
|
||
I'm concerned that it might affect Orca's knowledge of where the user is when the page loads.
Assignee | ||
Comment 4•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Attachment #289523 -
Flags: review?(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.
Attachment #289523 -
Flags: review?(ginn.chen) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #289523 -
Flags: approval1.9?
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•17 years ago
|
||
Comment on attachment 289523 [details] [diff] [review]
Partial fix. Needs testing to ensure it doesn't break Orca
a=beltzner on behalf of drivers
Attachment #289523 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 9•17 years ago
|
||
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•17 years ago
|
||
same patch with no mDocSelectionListeners counting
Attachment #289680 -
Attachment is obsolete: true
Attachment #290203 -
Flags: review?(aaronleventhal)
Attachment #289680 -
Flags: review?(ginn.chen)
Assignee | ||
Updated•17 years ago
|
Attachment #290203 -
Flags: review?(aaronleventhal)
Attachment #290203 -
Flags: review+
Attachment #290203 -
Flags: approval1.9?
Assignee | ||
Updated•17 years ago
|
Whiteboard: Endgame drivers: we want the last patch as well -- it's more of a core fix
Updated•17 years ago
|
Attachment #290203 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 11•15 years ago
|
||
I can't tell what happened to the approvals in this bug :(
Keywords: crash
Summary: Crash [ @ FireDelayedAccessibleEvent ] → Crash [@ FireDelayedAccessibleEvent ]
Updated•14 years ago
|
Crash Signature: [@ FireDelayedAccessibleEvent ]
You need to log in
before you can comment on or make changes to this bug.
Description
•