377470 - Crash [@ nsAccessible::GetState] using accessibility api and moving stuff

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

I'm marking this security sensitive, because this uses Mats' script from bug 376924 (thanks for that, Mats).
If it doesn't need to be security sensitive, please open the bug.

The testcase doesn't seem to crash on branch, so it seems like a regression.

Talkback ID: TB31175040E
nsAccessible::GetState  [mozilla/accessible/src/base/nsaccessible.cpp, line 1018]
nsHTMLComboboxListAccessible::GetState  [mozilla/accessible/src/html/nshtmlselectaccessible.cpp, line 1303]
nsAccessible::GetFinalState  [mozilla/accessible/src/base/nsaccessible.cpp, line 2329]
nsAccessible::State  [mozilla/accessible/src/base/nsaccessible.h, line 192]
nsAccessible::GetMultiSelectFor  [mozilla/accessible/src/base/nsaccessible.cpp, line 1346]
nsHTMLSelectOptionAccessible::SelectionChangedIfOption  [mozilla/accessible/src/html/nshtmlselectaccessible.cpp, line 761]
nsDocAccessible::ContentStatesChanged  [mozilla/accessible/src/base/nsdocaccessible.cpp, line 1039]
nsHTMLOptionElement::SetSelectedInternal  [mozilla/content/html/content/src/nshtmloptionelement.cpp, line 226]
nsHTMLSelectElement::OnOptionSelected  [mozilla/content/html/content/src/nshtmlselectelement.cpp, line 1226]
nsHTMLSelectElement::SetOptionsSelectedByIndex  [mozilla/content/html/content/src/nshtmlselectelement.cpp, line 1379]
nsHTMLSelectElement::SetSelectedIndex  [mozilla/content/html/content/src/nshtmlselectelement.cpp, line 1175]
nsHTMLSelectElement::RemoveOptionsFromList  [mozilla/content/html/content/src/nshtmlselectelement.cpp, line 732]
nsHTMLSelectElement::WillRemoveOptions  [mozilla/content/html/content/src/nshtmlselectelement.cpp, line 899]
nsHTMLSelectElement::RemoveChildAt  [mozilla/content/html/content/src/nshtmlselectelement.cpp, line 586]
nsGenericElement::InsertBefore  [mozilla/content/base/src/nsgenericelement.cpp, line 2537]
nsGenericElement::AppendChild  [mozilla/content/base/src/nsgenericelement.h, line 530]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2247]

Comment 1

[Martijn Wargers (dead)]

The testcase uses enhanced privileges, so you need to download it to your computer to see the crash.

Comment 2

[Mats Palmgren (inactive)]

Attachment: stack

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 4

[Aaron Leventhal]

Comment on attachment 261543 [details] [diff] [review]
Patch rev. 1

I haven't tested what this does to our implementation,  and whether we still return the right frame. Does it?

The constructor shouldn't  take aListFrame any more should it? What is it used for now?

Comment 5

[Aaron Leventhal]

Is the crash really because of bug 357583, and we're not getting notifications from layout because a11y is initialized by JS?

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Now returning the right frame...

Comment 7

[Aaron Leventhal]

Comment on attachment 261589 [details] [diff] [review]
Patch rev. 2

Thanks for the fix!

Just a nit, s/f/frame. I prefer multiple letter variable names, because it makes it easier to search for them in the code, and are slightly more readable IMO.

Also, the code does slightly less work if you do:

+  if (frame) {
+    nsIComboboxControlFrame* comboBox;
+    CallQueryInterface(f, &comboBox);
+    if (comboBox) {
+      return comboBox->GetDropDown();
+    }
+  }
+  return nsnull;
}

Comment 8

[Aaron Leventhal]

I checked it in with that change. Thanks for the fix Mats.

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to comment #0)
> The testcase doesn't seem to crash on branch, so it seems like a regression.

Crashed for me in a debug FF2.0.0.4pre build. kinda hung, then crashed when I started typing in the input field. Stack is different so could be something else (in which case we should create a branch-only bug). mFirstChild in nsAccessible::Shutdown points at an object with a deleted vtbl (0xdddddddd).

>	nsCOMPtr<nsIAccessible>::nsCOMPtr<nsIAccessible>() Line 627
 	nsAccessible::Shutdown() Line 455
 	nsAccessNode::ClearCacheEntry() Line 530
 	nsBaseHashtable<nsVoidHashKey,nsCOMPtr<nsIAccessNode>,nsIAccessNode *>::s_EnumStub() Line 346
 	PL_DHashTableEnumerate() Line 683
 	nsBaseHashtable<nsVoidHashKey,nsCOMPtr<nsIAccessNode>,nsIAccessNode *>::Enumerate() Line 221
 	nsAccessNode::ClearCache() Line 538
 	nsDocAccessible::Shutdown() Line 483
 	nsDocAccessibleWrap::Shutdown() Line 173
 	nsAccessNode::ClearCacheEntry() Line 530
 	nsBaseHashtable<nsVoidHashKey,nsCOMPtr<nsIAccessNode>,nsIAccessNode *>::s_EnumStub() Line 346
 	PL_DHashTableEnumerate() Line 683
 	nsBaseHashtable<nsVoidHashKey,nsCOMPtr<nsIAccessNode>,nsIAccessNode *>::Enumerate() Line 221
 	nsAccessNode::ClearCache() Line 538
 	nsAccessNode::ShutdownXPAccessibility() Line 214
 	nsAccessNodeWrap::ShutdownAccessibility() Line 588
 	nsAccessibilityService::Observe() Line 154
 	nsObserverService::NotifyObservers() Line 233
 	NS_ShutdownXPCOM_P() Line 812
 	ScopedXPCOMStartup::~ScopedXPCOMStartup() Line 742
 	XRE_main() Line 2743
 	main() Line 61
 	mainCRTStartup() Line 398

Comment 10

[Daniel Veditz [:dveditz]]

Created bug 386382 for the branch shutdown crash described in comment 9

Comment 11

[Daniel Veditz [:dveditz]]

This particular patch doesn't apply to the 1.8 branch; it tweaks some of the changes that were landed in bug 278034 which was post-1.8 only. Whatever is going on in bug 386382 is completely unrelated to what this bug is fixing, despite being discovered by the same testcase.