380412 - Loading AJAX pages reliable crashes Firefox if assistive technologies are enabled in GNOME [@ nsAccEvent::GetAccessible()]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070511 Minefield/3.0a5pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070511 Minefield/3.0a5pre

If assistive technologies are enabled in GNOME, whether or not any AT is actually running, loading certain AJAX pages will crash Firefox 100% of the time.  This issue began in the May 10th build.  Prior to that build the issue did not occur. If AT is not enabled, this issue does not occur.

Reproducible: Always

Steps to Reproduce:
1. Enable assistive technologies in GNOME.  If they were not enabled already, log out and back in.
2. Load either gmail.com or netvibes.com
Actual Results:  
Firefox crashes.

Expected Results:  
Firefox wouldn't crash.

Talkback incident example: http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=tb32039920x

Comment 1

[Steve England [:stevee]]

Incident ID: 32039920
Stack Signature	nsAccEvent::GetAccessible() 09448aed
Product ID	FirefoxTrunk
Build ID	2007051104
Trigger Time	2007-05-11 08:34:26.0
Platform	LinuxIntel
Operating System	Linux 2.6.20-15-generic
Module	firefox-bin + (00694e87)
URL visited	
User Comments	
Since Last Crash	0 sec
Total Uptime	0 sec
Trigger Reason	SIGSEGV: Segmentation Fault: (signal 11)
Source File, Line No.	/builds/tinderbox/Fx-Trunk-Cairo/Linux_2.4.21-32.0.1.EL_Depend/mozilla/accessible/src/base/nsAccessibleEventData.cpp, line 113
Stack Trace 	
nsAccEvent::GetAccessible()  [mozilla/accessible/src/base/nsAccessibleEventData.cpp, line 113]
nsDocAccessible::FlushPendingEvents()  [mozilla/accessible/src/base/nsDocAccessible.cpp, line 840]
nsDocAccessible::FlushEventsCallback()  [mozilla/accessible/src/base/nsDocAccessible.cpp, line 1299]
nsTimerImpl::Fire()  [mozilla/xpcom/threads/nsTimerImpl.cpp, line 384]
nsTimerEvent::Run()  [mozilla/xpcom/threads/nsTimerImpl.cpp, line 955]
nsThread::ProcessNextEvent()  [mozilla/xpcom/threads/nsThread.cpp, line 483]
NS_ProcessNextEvent_P()  [mozilla/xpcom/build/nsThreadUtils.cpp, line 227]
nsBaseAppShell::Run()  [mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp, line 154]
nsAppStartup::Run()  [mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 172]
XRE_main()  [mozilla/toolkit/xre/nsAppRunner.cpp, line 2808]
main()  [mozilla/browser/app/nsBrowserApp.cpp, line 62]
libc.so.6 + 0x15ebc (0xb72deebc)

Comment 2

[Aaron Leventhal]

Surkov, do you want to back out your fix for bug 377728 until the crashes are resolved?

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

can you try this patch?

Comment 4

[alexander :surkov (:asurkov)]

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

checked in, leaving open until fix is confirmed

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch 2.1

make methods of nsIAccessibleRetrieval more safe.

Comment 6

[Joanmarie Diggs]

(In reply to comment #4)
> (From update of attachment 264521 [details] [diff] [review])
> checked in, leaving open until fix is confirmed
> 
In today's build (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070512 Minefield/3.0a5pre), the problem seems to be solved.  Thanks!

I assume that "patch 2.1" didn't get included yet.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 264521 [details] [diff] [review] [details])
> > checked in, leaving open until fix is confirmed
> > 
> In today's build (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre)
> Gecko/20070512 Minefield/3.0a5pre), the problem seems to be solved.  Thanks!

Thank you. I'm happy :)

> I assume that "patch 2.1" didn't get included yet.
> 

Yes. "patch 2.1" is not bug fix actually. It is just a guarantee that code like from bug 377728 won't lead to crash.

Comment 8

[Aaron Leventhal]

Comment on attachment 264609 [details] [diff] [review]
patch 2.1

Please remove the whitespace-only changes. It's not worth the minor cleanup for changing CVS  history. We use CVS  all the time.

The order should be switched to be like this:

+  NS_ENSURE_ARG_POINTER(aAccessible);
+  *aAccessible = nsnull;
+
+  NS_ENSURE_ARG(aNode);  
+  NS_ENSURE_ARG(aPresShell);
+  

Te helps aAccessible to get cleared out in any case where the out param is not null.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch 2.2

for checking in

Comment 10

[alexander :surkov (:asurkov)]

checked in