488562 - Crash [@ nsRootAccessible::FireAccessibleFocusEvent] on reload with DOMAttrModified removing window and accessibility

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: zipped up testcase (uses enhanced privileges for accessibility code)

See zipped up testcase, which crashes current trunk build and Firefox3.0.7 (so marking security sensitive, just to be sure).

The testcase uses enhanced privileges for the accessible nodes retrieval.

To reproduce, open the file named 'parenframe.htm', then reload.

http://crash-stats.mozilla.com/report/index/1991cd32-c8ee-46b3-a19d-eba0b2090415?p=1
0  	xul.dll  	nsRootAccessible::FireAccessibleFocusEvent  	 accessible/src/base/nsRootAccessible.cpp:533
1 	xul.dll 	nsCOMPtr_base::assign_from_qi 	obj-firefox/xpcom/build/nsCOMPtr.cpp:96
2 	MSCTF.dll 	OnShellLanguage

Firefox3.0.7 crash stack:
http://crash-stats.mozilla.com/report/index/3a7ff6c7-3aeb-46f9-8675-e1bcd2090415?p=1
0  	xul.dll  	nsRootAccessible::FireAccessibleFocusEvent  	 mozilla/accessible/src/base/nsRootAccessible.cpp:528
1 	xul.dll 	nsRootAccessible::HandleEventWithTarget 	mozilla/accessible/src/base/nsRootAccessible.cpp:823
2 	xul.dll 	nsRootAccessible::HandleEvent 	mozilla/accessible/src/base/nsRootAccessible.cpp:591
3 	xul.dll 	nsEventListenerManager::HandleEventSubType 	mozilla/content/events/src/nsEventListenerManager.cpp:1080
4 	xul.dll 	nsEventListenerManager::HandleEvent 	mozilla/content/events/src/nsEventListenerManager.cpp:1185
5 	xul.dll 	nsEventTargetChainItem::HandleEventTargetChain 	mozilla/content/events/src/nsEventDispatcher.cpp:241
6 	xul.dll 	nsCOMPtr_base::assign_with_AddRef 	nsCOMPtr.cpp:88 

Perhaps this is related to bug 488505, where there is also a case where the accessibility code seems to set an attribute at an unsafe time or something like that?
Also, perhaps this is related to bug 422524?

Comment 1

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

Proposed fix in nsRootAccessible::FireAccessibleFocusEvent:

nsCOMPtr<nsIContent> focusContent = do_QueryInterface(gLastFocusedNode);
nsIFrame *focusFrame = nsnull;
if (focusContent) {
  nsCOMPtr<nsIPresShell> shell = nsAccessNode::GetPresShellFor(gLastFocusedNode);

+  if (!shell)
+    return PR_FALSE;

  focusFrame = shell->GetRealPrimaryFrameFor(focusContent);


Can someone compile and test this easily? I don't have a 1.9.0 tree handy.

Comment 2

[Martijn Wargers (dead)]

(In reply to comment #1)
> Can someone compile and test this easily? I don't have a 1.9.0 tree handy.

It is also crashing on trunk.

Comment 3

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

OK (I was thrown by bug 422524 comment 1)

Comment 4

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

Adding a dependency (bug 423737) so we can possibly undo any workaround here when that bug resolves.

Comment 5

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

Attachment: bailout fix

Martijn, this patch fixes the bug locally, can you confirm?

Comment 6

[alexander :surkov (:asurkov)]

David, does it makes sense to keep gLastFocusedNode if it hasn't presshell?

Comment 7

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

Attachment: bailout fix 2

(In reply to comment #6)
> David, does it makes sense to keep gLastFocusedNode if it hasn't presshell?

Nope. Thanks Alex, what do you think of this patch?

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 373351 [details] [diff] [review]
bailout fix 2

I don't understand why fix #1 doesn't fix the problem, but fix #2 does. Is it stacktrace after fix #1 is different than original one? Does it related we save node without presshell in gLastFocusNode?

I would like to see an assertion to assert when presshell is null. It's not expected I think.

But the patch looks safe, r=me.

Comment 9

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

Attachment: patch to go in (includes assertion)

Comment 10

[Marco Zehe (:MarcoZ)]

Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/370bac6b036d