Firefox 3.6 failed to start with AT-SPI2 0.1.3

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

Tracking

1.9.2 Branch
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

AT-SPI2 is doing update_tree for some signals.

In atk nsApplicationAccessibleWrap::AddRootAccessible() we do g_signal_emit_by_name() for "children_changed:add" of the new root accessible.
Then AT-SPI2 will do atk_object_get_n_accessible_children for the new added root accessible.

But we didn't finish nsRootAccessible::Init() yet.

So we will crash with stack overflow.

I think we can fix it by firing the event through FireDelayedAccessibleEvent().

The bug is not reproducible with Firefox 3.5, I don't know why.

stack:

 fe213711 unsigned nsApplicationAccessibleWrap::AddRootAccessible(nsIAccessible*) (efa029c0, efcacd90, 804334c, fe1d5732) + 89
 fe1d5775 unsigned nsRootAccessible::Init() (efcacd80, efcacd90, f6938400, 0) + 51
 fe1b1a79 unsigned nsAccessibilityService::CreateRootAccessible(nsIPresShell*,nsIDocument*,nsIAccessible**) (efcff8e0, f4e86460, f5e81000, 8043470) + 399
 fe1ba831 unsigned nsAccessibilityService::GetAccessible(nsIDOMNode*,nsIPresShell*,nsIWeakReference*,nsIFrame**,int*,nsIAccessible**) (efcff8e0, f5e8109c, f4e86460, f6938400, 80437ac, 804
37b0) + 22b5
 fe1b8188 unsigned nsAccessibilityService::GetAccessibleInShell(nsIDOMNode*,nsIPresShell*,nsIAccessible**) (efcff8e0, f5e8109c, f4e86460, 8043808) + 74
 fe18b8d5 unsigned nsAccessNode::Init() (efa0d180, efa0d190, 8043874, fe1b82e6) + 155
 fe1b831f unsigned nsAccessibilityService::InitAccessible(nsIAccessible*,nsIAccessible**,nsRoleMapEntry*) (efcff8e0, efa0d190, 8043d38, 0) + 47
 fe1ba2b6 unsigned nsAccessibilityService::GetAccessible(nsIDOMNode*,nsIPresShell*,nsIWeakReference*,nsIFrame**,int*,nsIAccessible**) (efcff8e0, f60b559c, f4e86460, f6938400, 8043c20, 804
3d58) + 1d3a
 fe1cd001 int nsAccessibleTreeWalker::GetAccessible() (8043d34, f60b556c, f4e86460, fe1ccc1e) + b9
 fe1ccca2 unsigned nsAccessibleTreeWalker::GetFirstChild() (8043d34, 0, f4e86460, fe1ccc1e) + 92
 fe1cccb2 unsigned nsAccessibleTreeWalker::GetFirstChild() (8043d34, 0, 8043cd4, fe1ccc1e) + a2
 fe1cccb2 unsigned nsAccessibleTreeWalker::GetFirstChild() (8043d34, f4ec1628, f5e8109c, 1) + a2
 fe1c0404 void nsAccessible::CacheChildren() (efcaccc0, 8043db8, 8043dbc, fe20093a) + 94
 fe20098b void nsHyperTextAccessible::CacheChildren() (efcaccc0, efa0b984, efa029c0, 0) + 5f
 fe1c0523 unsigned nsAccessible::GetChildCount(int*) (efcaccc0, 8043e78, 8043e80, fe20ce1a) + 17
 fe20cee5 getChildCountCB (efa0cd30, efcfcc70, 8043ebc, fb29b83c) + d9
 fb29b861 atk_object_get_n_accessible_children (efa0cd30, 0) + 3d
 fa97208c append_children (efa0cd30, efa05dc0) + 2c
 fa9721f8 register_subtree (efa0cd30, 0) + f8
 fa972eec tree_update_children_action (8044060, 3, efa0d080, f45debe0, efa07310, 0) + bc
 fa9729fe tree_update_wrapper (8044060, 3, efa0d080, f45debe0, fa972e30, 0) + ce
 fa972f7f tree_update_children_listener (8044060, 3, efa0d080, f45debe0) + 3f
 fc38599f signal_emit_unlocked_R (efa07070, 1ed, efa07310, 0, efa0d080, efa07310) + b07
 fc384a88 g_signal_emit_valist (efa07310, d8, 1ed, 8044270) + 9c8
 fc384de2 g_signal_emit_by_name (efa07310, fe535cc9, 0, efa0cd30, 0, 8044294) + 14e
 fe213711 unsigned nsApplicationAccessibleWrap::AddRootAccessible(nsIAccessible*) (efa029c0, efcaccd0, 80442f0, fe1d5732) + 89
 fe1d5775 unsigned nsRootAccessible::Init() (efcaccc0, efcaccd0, f6938400, 0) + 51
 fe1b1a79 unsigned nsAccessibilityService::CreateRootAccessible(nsIPresShell*,nsIDocument*,nsIAccessible**) (efcff8e0, f4e86460, f5e81000, 8044414) + 399
 fe1ba831 unsigned nsAccessibilityService::GetAccessible(nsIDOMNode*,nsIPresShell*,nsIWeakReference*,nsIFrame**,int*,nsIAccessible**) (efcff8e0, f5e8109c, f4e86460, f6938400, 8044750, 804
4754) + 22b5
 fe1b8188 unsigned nsAccessibilityService::GetAccessibleInShell(nsIDOMNode*,nsIPresShell*,nsIAccessible**) (efcff8e0, f5e8109c, f4e86460, 80447b8) + 74
 fd4b01af unsigned PresShell::HandleEventInternal(nsEvent*,nsIView*,nsEventStatus*) (f4e86460, 8044958, 0, 8044878) + b7
 fd4b003a unsigned PresShell::HandleEventWithTarget(nsEvent*,nsIFrame*,nsIContent*,nsEventStatus*) (f4e86460, 8044958, 0, 0, 8044878, 80448c0) + f6
 fdec49de nsEventStatus nsWebShellWindow::HandleEvent(nsGUIEvent*) (8044958, 1000000, 0, fe12b896) + 7da
Assignee: nobody → ginn.chen
Severity: normal → critical
I think we may not be able to reuse FireDelayedAccessibleEvent() because aRootAccWrap may not be nsDocAccessible and it may not have mDOMNode.
We have nsNativeRootAccessibleWrap for ATK.
Ginn, is AT-SPI2 default on current distros? -- just trying to get a sense of the urgency here.
AFAIK, it's targeting GNOME 2.30.
Besides this won't startup bug, Firefox is still unstable with AT-SPI2.
I think we don't need to rush for it.
Posted patch patch (obsolete) — Splinter Review
Just move AddRootAccessible under nsDocAccessibleWrap::Init will fix it.
I think we can use this easy fix for now.

Also move g_object_ref(childAtkObj); before atk_object_set_parent(childAtkObj, aAtkObj);
childAtkObj might be destroyed during atk_object_set_parent().

BTW: There're other bugs/crashes of Firefox with AT-client open under AT-SPI2.
But I think they're likely bugs of AT-client or AT-SPI2.
I'll work with GNOME developers on them.
Attachment #417045 - Flags: review?(bolterbugz)
Comment on attachment 417045 [details] [diff] [review]
patch

r=me thanks!

Ginn, thanks for watching and testing with atspi2 -- this is a huge help.
Attachment #417045 - Flags: review?(bolterbugz) → review+
http://hg.mozilla.org/mozilla-central/rev/08e208698ef0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Backed out due to tinderbox orange (mac/linux at least):
1001 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_events_doc.html | Test timed out.
1011 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_events_draganddrop.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - nsIAccessibleEvent is not defined at chrome://mochikit/content/a11y/accessible/events.js:766

And additional errors and leaks. See http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1260970705.1260973750.17801.gz for a log.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I confirmed locally that this was the push causing the orange.

We must connect the accessible object to the one true application root before calling nsDocAccessibleWrap::Init.
For next time, let's make sure one of us (author or reviewer) runs patches through our test suite before pushing :)
Looking deeper, nsDocAccessible::Init expects the current document to have a parent. It also adds listeners, and fires a reorder event.
Can we delay the g_signal_emit_by_name as per comment #0?
Option 1)
We can use a timer to delay g_signal_emit_by_name().

Option 2)
We can do

root->AddRootAccessible(this);
nsresult rv = nsDocAccessibleWrap::Init();
#ifdef MOZ_ACCESSIBILITY_ATK
if (NS_SUCCEEDED(rv))
    root->FireAddRootAccessibleEvent(this);
#endif

------
I'd prefer option 2). I don't like delayed event, it would cause more trouble.
But the code is a bit odd.
I mean
a) We don't want #ifdef MOZ_ACCESSIBILITY_ATK in base. We also don't want to add functions to every platform nsApplicationAccessibleWrap.
b) We need FireAddRootAccessibleEvent(), but we don't need FireRemoveRootAccessibleEvent().

Comments?

However, I don't have access to the AT-SPI2 machine until Friday.
I can do regression test first. :)
Status: REOPENED → ASSIGNED
Alexander what are your thoughts? I think we could try option 1 or go with option 2 but probably without the ifdef (use wraps).

Ginn, note we will be cleaning up (reorganizing) our event code probably in Q1. So any ugliness now is probably temporary.
Posted patch patch v2Splinter Review
use g_timeout_add to emit the event
remove the useless logs

verified with at-spi2 and mochitest-a11y on Solaris.
Attachment #417045 - Attachment is obsolete: true
Attachment #418350 - Flags: review?(bolterbugz)
Attachment #418350 - Flags: review?(surkov.alexander)
Comment on attachment 418350 [details] [diff] [review]
patch v2

>+++ b/accessible/src/atk/nsApplicationAccessibleWrap.cpp
>@@ -630,72 +630,77 @@ nsApplicationAccessibleWrap::GetNativeIn
>         mAtkObject->role = ATK_ROLE_INVALID;
>         mAtkObject->layer = ATK_LAYER_INVALID;
>     }
> 
>     *aOutAccessible = mAtkObject;
>     return NS_OK;
> }
> 
>+struct AtkAddRootAccessibleEvent {

Nit: I'd prefer AtkRootAccessibleAddedEvent.

>+  AtkObject *app_accessible;
>+  AtkObject *root_accessible;
>+  PRUint32 index;
>+};

>+gboolean fireAddRootAccessibleCB(gpointer data)

and nit: fireRootAccessibleAddedCB

A11y tests pass on Mac as well.
Attachment #418350 - Flags: review?(bolterbugz) → review+
Could we fire EVENT_SHOW for the new document so that ATK's nsAccessibleWrap event handling will do all work for us?
(In reply to comment #16)
> Could we fire EVENT_SHOW for the new document so that ATK's nsAccessibleWrap
> event handling will do all work for us?

It doesn't work for the nsNativeRootAccessibleWrap case.
Comment on attachment 418350 [details] [diff] [review]
patch v2

Sounds good to me. However I have couple of questions.

1. Why do you not use Mozilla's version of timeout call (like NS_DispatchToMainThread) instead of g_timeout_add?
2. Why do you not need  MAI_LOG_DEBUG any more?
Attachment #418350 - Flags: review?(surkov.alexander) → review+
NS_DispatchToMainThread should work as well.
I think g_timeout_add is lighter and we only use it with ATK.

I never use MAI_LOGGING, I don't think it's useful for AddRootAccessible/RemoveRootAccessible.
Also it is already broken, i.e.
#ifdef MAI_LOGGING
    if (NS_SUCCEEDED(rv)) {
after
NS_ENSURE_SUCCESS(rv, rv);

I don't want to fix it. Instead I dump it.
Ok, it's up to you.
http://hg.mozilla.org/mozilla-central/rev/0e6f4f57a0a1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
For those who need to run Firefox 3.6 on GNOME 2.29.

AT-SPI2 is enabled as default in GNOME 2.29.
AFAIK, AT-SPI (CORBA version) is the default for GNOME 2.30.
Hmm I wonder if there is a way to detect at-spi2 at runtime and do the right thing based on that detection?
(In reply to comment #23)
> Hmm I wonder if there is a way to detect at-spi2 at runtime and do the right
> thing based on that detection?

There's no interface to get the information from application side.
You need to log in before you can comment on or make changes to this bug.