608201 - File chooser and some other dialogs are not added to Firefox a11y tree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Ginn Chen]

Open Firefox, open file chooser dialog
You can't see file chooser dialog in accerciser.

The problem is these dialog are using gtk_dialog_run() directly, not the a11y helper.

It can be easily fixed by replacing gtk_dialog_run with RunDialog.

But I get crashes with these dialogs on exit Firefox now, might be another bug.

Comment 1

[Ginn Chen]

The  root cause of the crash is nsAccessible::RemoveChild()

We do RemoveElementAt() first, aChild might be deconstructed at this point, then aChild->UnindFromParent() is doing random things, I guess.

Comment 2

[Ginn Chen]

Attachment: patch

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #0)

> But I get crashes with these dialogs on exit Firefox now, might be another bug.

(In reply to comment #1)
> The  root cause of the crash is nsAccessible::RemoveChild()

might be bug 606962?

> We do RemoveElementAt() first, aChild might be deconstructed at this point,
> then aChild->UnindFromParent() is doing random things, I guess.

could add a stack here?

Comment 4

[Ginn Chen]

(In reply to comment #3)
> > The  root cause of the crash is nsAccessible::RemoveChild()
> 
> might be bug 606962?

I think it might cause crashes like Bug 606962.

> 
> > We do RemoveElementAt() first, aChild might be deconstructed at this point,
> > then aChild->UnindFromParent() is doing random things, I guess.
> 
> could add a stack here?

Same parent (nsApplicationAccessible) is released twice for the same child.

=>[1] nsApplicationAccessible::Release(this = 0x8f848f8), line 64 in "nsApplicationAccessible.cpp"
  [2] nsRefPtr<nsAccessible>::~nsRefPtr(this = 0x945814c), line 969 in "nsAutoPtr.h"
  [3] nsAccessible::~nsAccessible(this = 0x9458128), line 217 in "nsAccessible.cpp"
  [4] nsAccessibleWrap::~nsAccessibleWrap(this = 0x9458128), line 286 in "nsAccessibleWrap.cpp"
  [5] nsHyperTextAccessible::~nsHyperTextAccessible(0x9458128, 0x0), at 0xfcb8b0a1
  [6] nsDocAccessible::~nsDocAccessible(this = 0x9458128), line 112 in "nsDocAccessible.cpp"
  [7] nsDocAccessibleWrap::~nsDocAccessibleWrap(this = 0x9458128), line 58 in "nsDocAccessibleWrap.cpp"
  [8] nsRootAccessible::~nsRootAccessible(this = 0x9458128), line 118 in "nsRootAccessible.cpp"
  [9] nsNativeRootAccessibleWrap::~nsNativeRootAccessibleWrap(this = 0x9458128), line 53 in "nsRootAccessibleWrap.cpp"
  [10] __SLIP.DELETER__B(0x9458128, 0x1), at 0xfcc1dacc
  [11] nsAccessNode::LastRelease(this = 0x9458128), line 134 in "nsAccessNode.cpp"
  [12] nsAccessNode::Release(this = 0x9458128), line 107 in "nsAccessNode.cpp"
  [13] nsAccessible::Release(this = 0x9458128), line 133 in "nsAccessible.cpp"
  [14] nsHyperTextAccessible::Release(this = 0x9458128), line 86 in "nsHyperTextAccessible.cpp"
  [15] nsDocAccessible::Release(this = 0x9458128), line 174 in "nsDocAccessible.cpp"
  [16] nsRootAccessible::Release(this = 0x9458128), line 105 in "nsRootAccessible.cpp"
  [17] nsRefPtr<nsAccessible>::~nsRefPtr(this = 0x8f8cd0c), line 969 in "nsAutoPtr.h"
  [18] nsTArrayElementTraits<nsRefPtr<nsAccessible> >::Destruct(e = 0x8f8cd0c), line 204 in "nsTArray.h"
  [19] nsTArray<nsRefPtr<nsAccessible> >::DestructRange(this = 0x8f84920, start = 1U, count = 1U), line 987 in "nsTArray.h"
  [20] nsTArray<nsRefPtr<nsAccessible> >::RemoveElementsAt(this = 0x8f84920, start = 1U, count = 1U), line 718 in "nsTArray.h"
  [21] nsTArray<nsRefPtr<nsAccessible> >::RemoveElementAt(this = 0x8f84920, index = 1U), line 724 in "nsTArray.h"
  [22] nsAccessible::RemoveChild(this = 0x8f848f8, aChild = 0x9458128), line 2780 in "nsAccessible.cpp"
  [23] nsApplicationAccessibleWrap::RemoveChild(this = 0x8f848f8, aChild = 0x9458128), line 693 in "nsApplicationAccessibleWrap.cpp"
  [24] nsAccessibilityService::RemoveNativeRootAccessible(this = 0x8f82f38, aAccessible = 0x9458128), line 1780 in "nsAccessibilityService.cpp"
  [25] RunDialog(aDialog = 0x9444040), line 69 in "nsAccessibilityHelper.cpp"
  [26] nsFilePicker::Show(this = 0x9028b88, aReturn = 0x8042470), line 541 in "nsFilePicker.cpp"

Note: Although mParent is released on deconstruct of nsNativeRootAccessibleWrap, mParent is not setting null.

=>[1] nsApplicationAccessible::Release(this = 0x8f848f8), line 64 in "nsApplicationAccessible.cpp"
  [2] nsRefPtr<nsAccessible>::assign_assuming_AddRef(this = 0x945814c, newPtr = (nil)), line 957 in "nsAutoPtr.h"
  [3] nsRefPtr<nsAccessible>::assign_with_AddRef(this = 0x945814c, rawPtr = (nil)), line 941 in "nsAutoPtr.h"  [4] nsRefPtr<nsAccessible>::operator=(this = 0x945814c, rhs = (nil)), line 1025 in "nsAutoPtr.h"
  [5] nsAccessible::UnbindFromParent(this = 0x9458128), line 2713 in "nsAccessible.cpp"  [6] nsAccessible::RemoveChild(this = 0x8f848f8, aChild = 0x9458128), line 2783 in "nsAccessible.cpp"
  [7] nsApplicationAccessibleWrap::RemoveChild(this = 0x8f848f8, aChild = 0x9458128), line 693 in "nsApplicationAccessibleWrap.cpp"
  [8] nsAccessibilityService::RemoveNativeRootAccessible(this = 0x8f82f38, aAccessible = 0x9458128), line 1780 in "nsAccessibilityService.
cpp"  [9] RunDialog(aDialog = 0x9444040), line 69 in "nsAccessibilityHelper.cpp"
  [10] nsFilePicker::Show(this = 0x9028b88, aReturn = 0x8042470), line 541 in "nsFilePicker.cpp"

mParent is released again because we're operating on wild memory.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 486873 [details] [diff] [review]
patch


> PRBool
> nsAccessible::InsertChildAt(PRUint32 aIndex, nsAccessible* aChild)
> {

I think it's worth to add precondition for aChild in InsertChildAt/AppendChild (and RemoveChild?) per timeless commment in bug 606962. We don't call these methods with null aChild, we need check null check. But perhaps it's correct from API point of view.

> 
>-  if (aChild->mIndexInParent >= mChildren.Length() ||
>-      mChildren[aChild->mIndexInParent] != aChild) {
>+  if (index >= mChildren.Length() ||
>+      mChildren[index] != aChild) {

place them on the same line

Comment 6

[alexander :surkov (:asurkov)]

Ginn, it sounds you need one more review for widget part.

Comment 7

[alexander :surkov (:asurkov)]

Can we get blocking beta 7? it's not file chooser dialogs problem only which is very important, this is crash fix as well.

Comment 8

[Ginn Chen]

I agree, we should get it into beta 7.

Comment 9

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

Note to drivers. This includes a cross platform crash fix but I probably shouldn't make the beta7 blockage call here. I understand the crash sig is:
http://crash-stats.mozilla.com/report/index/c306a3a0-6054-475f-a594-f3dee2101023

Comment 10

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

Setting as beta8+ since I'm not a beta7 driver.

Comment 11

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/c6310223480b

Comment 12

[Mark Banner (:standard8)]

This bug just broke the build:

/builds/slave/mozilla-central-linux-debug/build/accessible/src/base/nsAccessible.cpp:2734: error: 'PR_FLASE' was not declared in this scope
/builds/slave/mozilla-central-linux-debug/build/accessible/src/base/nsAccessible.cpp: In member function 'virtual PRBool nsAccessible::InsertChildAt(PRUint32, nsAccessible*)':
/builds/slave/mozilla-central-linux-debug/build/accessible/src/base/nsAccessible.cpp:2750: error: 'PR_FLASE' was not declared in this scope
/builds/slave/mozilla-central-linux-debug/build/accessible/src/base/nsAccessible.cpp:2756: warning: comparison between signed and unsigned integer expressions
/builds/slave/mozilla-central-linux-debug/build/accessible/src/base/nsAccessible.cpp: In member function 'virtual PRBool nsAccessible::RemoveChild(nsAccessible*)':
/builds/slave/mozilla-central-linux-debug/build/accessible/src/base/nsAccessible.cpp:2773: error: 'PR_FLASE' was not declared in this scope


PR_FLASE ?

Comment 13

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/09a2c950dae3

Sorry for the typo.