423176 - Crash with jaws 7.1 [@ jhook.dll@0x5311]

Status: VERIFIED FIXED

(Firefox :: Disability Access, defect)

Description

[Xavier Robin]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.8.1.12) Gecko/20080207 Firedragon/7.10 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031506 Minefield/3.0b5pre

Firefox crashes when selecting (by mouse clicking or keyboard) a drop-down select  list when (<select>) jaws is running.

Sometimes it also crashes on startup or when hovering the place selection list under the address bar or even when starting with the profile manager, but these are less reproducible.

Reproducible: Always

Steps to Reproduce:
0. (Start jaws if necessary)
1. Go to http://wikipedia.org (or any site with a drop-down select list)
2. Click on the drop-down select list (that with all languages choices)
Actual Results:  
Crash

Expected Results:  
No crash

No more crash after stopping jaws: firefox is stable. And it starts crashing again as soon as jaws is restarted.
The Windows (XP SP2) version is in french.

Several crash reports have been submitted with "jaws" in comment, but I can't find them in the crash tracking system.

Comment 1

[Xavier Robin]

Might be related to bug 420960, but it doesn't crash on the given examples at dojotoolkit.org.

Comment 2

[Steve England [:stevee]]

Xavier, does breakpad not appear? Having a crash stack would certainly help.

Comment 3

[Xavier Robin]

Yes it does, but I didn't understand how to view it (didn't know the about:crashes trick). Sorry.

So here it is: http://crash-stats.mozilla.com/report/index/26f6b919-f2ca-11dc-ba83-001a4bd43e5c

jhook.dll@0x5311

I have a bunch of reports if it can help, I think they are all here : http://crash-stats.mozilla.com/?do_query=1&product=Firefox&version=Firefox%3A3.0b5pre&platform=windows&query_search=signature&query_type=contains&query=jhook&date=&range_value=1&range_unit=weeks

Comment 4

[Steve England [:stevee]]

Signature	jhook.dll@0x5311
UUID	26f6b919-f2ca-11dc-ba83-001a4bd43e5c
Time	2008-03-15 12:57:11-07:00
Uptime	0
Product	Firefox
Version	3.0b5pre
Build ID	2008031506
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 11
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x1d03e4
Comments	

Frame  	Signature  	Source
0 	jhook.dll@0x5311 	
1 	jhook.dll@0x4a2a 	
2 	jhook.dll@0x9898 	
3 	jhook.dll@0x7afd 	
4 	__ClientCallWinEventProc 	
5 	KiUserCallbackDispatcher 	
6 	jhook.dll@0x7006 	
7 	nsHyperTextAccessibleWrap::FireAccessibleEvent(nsIAccessibleEvent*) 	mozilla/accessible/src/msaa/nsHyperTextAccessibleWrap.cpp:77
8 	nsDocAccessible::FlushPendingEvents() 	mozilla/accessible/src/base/nsDocAccessible.cpp:1671
9 	nsDocAccessible::FlushEventsCallback(nsITimer*, void*) 	mozilla/accessible/src/base/nsDocAccessible.cpp:1701
10 	nsTimerImpl::Fire() 	mozilla/xpcom/threads/nsTimerImpl.cpp:400
11 	nsTimerEvent::Run() 	mozilla/xpcom/threads/nsTimerImpl.cpp:490
12 	nsThread::ProcessNextEvent(int, int*) 	mozilla/xpcom/threads/nsThread.cpp:510
13 	nsThreadManager::GetCurrentThread() 	mozilla/xpcom/threads/nsThreadManager.cpp:199
14 	wmain 	mozilla/toolkit/xre/nsWindowsWMain.cpp:87
15 	firefox.exe@0x217f 	
16 	BaseProcessStart

Comment 5

[Aaron Leventhal]

Xavier, do you have time to download and try Firefox 3 beta 4? You can install it in a separate directory, and can launch it as long as Firefox 2 is closed. It's available here: http://www.mozilla.com/en-US/firefox/3.0b4/releasenotes/

Comment 6

[Marco Zehe (:MarcoZ)]

Also, what JAWS version were you using?

Comment 7

[Xavier Robin]

(In reply to comment #5)
> Xavier, do you have time to download and try Firefox 3 beta 4? You can install
> it in a separate directory, and can launch it as long as Firefox 2 is closed.
> It's available here: http://www.mozilla.com/en-US/firefox/3.0b4/releasenotes/

In fact the bug was first discovered in beta 4 before I tested in the latest nightly. Some reports are from Beta 4 like this one: http://crash-stats.mozilla.com/report/index/3b8c2a17-f204-11dc-be36-001a4bd43ed6
I just tested in Firefox 3 Beta 1 and I see the crashes there too, so it is not a recent regression.

(In reply to comment #6)
> Also, what JAWS version were you using?

I should have mentioned it in the first message obviously. It is version 7.10.506 Unicode with the default Firefox script.

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to comment #7)
> (In reply to comment #6)
> > Also, what JAWS version were you using?
> I should have mentioned it in the first message obviously. It is version
> 7.10.506 Unicode with the default Firefox script.

Can you test with 8 or 9 to see if you can reproduce the crash there, too? JAWS 7.10 has not been tested with Firefox 3.

Comment 9

[Marco Zehe (:MarcoZ)]

In addition, I cannot reproduce the problem with JAWS 8.

Comment 10

[Xavier Robin]

We could not reproduce this bug with JAWS Version 9.0.552U. Therefore I believe it is a problem with JAWS 7.10.

Should this bug be marked as invalid? Due to the high licensing cost of JAWS, a lot of people continue to use older versions of JAWS such as 7.10 and might not be able to upgrade...

Comment 11

[Marco Zehe (:MarcoZ)]

Xavier, I am actually not sure we *can* fix the problem on our end. jhook.dll is a JAWS module that reacts badly to a response it gets from us, and which Freedom Scientific aparently fixed on their end in 8.0.

Comment 12

[timeless]

marco: do we know what response we're sending is bad and how? if we can recognize that a certain version of jaws is running (we can do this if necessary) and we knew what not to send them, we could....

Comment 13

[Aaron Leventhal]

Also, upgrading JAWS might cost, but upgrading Firefox doesn't.

At this point I'd recommend going to Firefox 3 beta 4 right away for JAWS users out there, and then grabbing the final when it's ready.

Marco would you agree?

Comment 14

[Marco Zehe (:MarcoZ)]

(In reply to comment #13)
> At this point I'd recommend going to Firefox 3 beta 4 right away for JAWS users
> out there, and then grabbing the final when it's ready.

True, but the problem is that it also crashes with Firefox 3.0b4 and b5pre. The problem is in JAWS 7.10's jhook.dll.
I don't know enough about injecting a DLL into a process, but if it were possible to find out what causes JAWS to go bad, and if we could ask the injected DLL for its version number, then we could possibly fix it.

Comment 15

[Aaron Leventhal]

I don't even know what event we're firing to cause JAWS to crash. Since they're in process it brings us down as well.

Comment 16

[timeless]

marco: should be possible to get a processhandle for a library
http://msdn2.microsoft.com/en-us/library/ms683199(VS.85).aspx
jhookhandle=GetModuleHandle("jhook")
if (!jhookhandle) return;
http://msdn2.microsoft.com/en-us/library/ms683197(VS.85).aspx
GetModuleFileName(jhookhandle, &jhookpath, sizeof(jhookpath))
jhookpath[sizeof(jhookpath)-1]=0;
http://msdn2.microsoft.com/en-us/library/ms647005(VS.85).aspx
yourstring.SetLength(GetFileVersionInfoSize(jhookpath, &ignored));
http://msdn2.microsoft.com/en-us/library/ms647003(VS.85).aspx
GetFileVersionInfo(jhookpath, ignored, yourstring.Length(), yourstring.BeginWriting());
http://msdn2.microsoft.com/en-us/library/ms647464(VS.85).aspx
VerQueryValue(/*this is left as an exercise*/)

xavier: http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg
we're going to need a .dump

I typically suggest:
 .dump /maipwd /u /ba /c "comment" file

replace comment with something mentioning this bug number+jaws, and change file to something like c:\minefield-jaws-crash

you're going to need space to host that file for someone to retrieve it.

alternatively, if you can catch this crash in windbg and visit irc.mozilla.org and ask for people mentioned in this bug, someone can arrange to do live debugging.

Comment 17

[Xavier Robin]

I downloaded, installed and run WinDbg.
Firefox version was : Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032005 Minefield/3.0b5pre

I opened the executable with debug child processes checked. Then 

> .sympath SRV*c:\symbols*http://symbols.mozilla.org/firefox

> .symfix+ c:\symbols

> .reload /f

(as indicated in the tutorial)

Then debug->Go, firefox started, Alt-d, wikipedia.org, enter, tab, space (form mode on), down.
Firefox didn't close in windbg, so I did a Debug->Break. Firefox window was still open. Then

> .dump /maipwd /u /ba /c "bug 423176 jaws keaboard" c:\dump_keyboard

Complete windbg console output is here: http://home.etu.unige.ch/~robin0/temp/windbg.log

Finally I had to kill Firefox window that was still here.

Complete dump: http://home.etu.unige.ch/~robin0/temp/dump_keyboard_093c_2008-03-21_17-06-44-078_008c.dmp
sha1: d2a27723803f8e0e5cee0cbec96f42db299ae24e

I hope that all was done correctly and that it will help.

Comment 18

[timeless]

so the entrypoint it took was of course:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp&rev=1.114&mark=1705#1645
   NotifyWinEvent(winEvent, hWnd, OBJID_CLIENT, childID);

xul!nsHyperTextAccessibleWrap::FireAccessibleEvent+0xae
aEvent	0x026fe340 class nsIAccessibleEvent *
mIsFromUserInput	1
mEventType	8 /* nsIAccessibleEvent::EVENT_FOCUS */
mEventRule	eRemoveDupes (2)
mAccessible	0x032fcbe0 class nsISupports * [class nsHTMLSelectOptionAccessible]
mDOMNode	0x02f5fce8 class nsISupports * [class nsHTMLOptionElement]
	

1663   PRUint32 winEvent = gWinEventMap[eventType];
winEvent = 0x8005

1675   if (eventType == nsIAccessibleEvent::EVENT_TEXT_CARET_MOVED ||
1676       eventType == nsIAccessibleEvent::EVENT_FOCUS) {
1677     UpdateSystemCaret();

 221 NS_IMETHODIMP nsAccessNode::GetUniqueID(void **aUniqueID)
 223   *aUniqueID = static_cast<void*>(mDOMNode);

0x60cff3c0 class nsISupports *

1712 PRInt32 nsAccessibleWrap::GetChildIDFor(nsIAccessible* aAccessible)
1727   return - NS_PTR_TO_INT32(uniqueID);

-1624241088

I'm not tracing hwnd, someone else can, but it won't be that valuable.

jhook!Ordinal1:
15003fc8 8b442404        mov     eax,dword ptr [esp+4]
15006f47 c3              ret
15007afe jhook!Ordinal1+0x3b36
15007d67 c21c00          ret     1Ch
15007dac c3              ret

other tidbits:

!dlls -v
0x00193f30: C:\Program Files\Freedom Scientific\JAWS\7.10\jhook.dll
      Base   0x15000000  EntryPoint  0x150112cf  Size        0x00040000
      Flags  0x80084004  LoadCount   0x00000001  TlsIndex    0x00000000
             LDRP_IMAGE_DLL
             LDRP_ENTRY_PROCESSED
             LDRP_PROCESS_ATTACH_CALLED
      Company Name       Freedom Scientific BLV Group, LLC
      Product Name       JAWS
      Product Version    7, 10, 506, 7
      Original Filename  JHook.dll
      File Description   Helper file for JAWS
      File Version       7, 10, 506, 7

0x00195ac0: C:\Program Files\Freedom Scientific\Shared\FsDomSrv\10.12\FSDomNodeFirefox.DLL
      Base   0x02db0000  EntryPoint  0x02dbbebd  Size        0x00016000
      Flags  0x80284004  LoadCount   0x00000001  TlsIndex    0x00000000
             LDRP_IMAGE_DLL
             LDRP_ENTRY_PROCESSED
             LDRP_PROCESS_ATTACH_CALLED
             LDRP_IMAGE_NOT_AT_BASE
      Product Name        FSDomNod Dynamic Link Library
      Product Version    10, 16, 1681, 0
      Original Filename  FSDomNod.dll
      File Description   FSDomNod Dynamic Link Library
      File Version       10, 16, 1681, 0

So we can definitely find out if the creature is floating around and try to work with it (or around it, or not at all).

reporter: if you could produce the smallest testcase that triggers this, that'd be good. (save the crashing html file to disk, load it, crash, open file in text editor, delete half the file, save, load it again, if it doesn't crash, undo the delete and delete some other block)

I'm mostly wondering if:

data:text/html,<select><option>1</select> is sufficient, or whether you actually need some unicode characters in the list or ....

also, your system is French, if it's at all possible to install jaws7.1 on an en-us version of windows, that'd be good.

if someone's really interested, eventually they should write a small msaa only testcase which provokes the crash, that'd enable us to figure out exactly what jaws 7.1 tolerates. but i think we could probably just provide less information about select boxes, or possibly not in response to clicks or ....

Comment 19

[timeless]

oh right, is there an msaa spy that's capable of recording and reproducing an msaa session? such a thing would of course be wonderful:

spy.dll[proxy-recorder] => firefox; jaws => spy.exe[proxy-play]; 
ideally jaws => spy should crash. when it does, you should be able to use:
jaws => spy.exe[replay] to crash again, and then you could try removing elements from the recording to see if you can convince jaws not to crash.

Comment 20

[Xavier Robin]

> I'm mostly wondering if:
> 
> data:text/html,<select><option>1</select> is sufficient, or whether you
> actually need some unicode characters in the list or ....

Yes it is.
<select><option></select> as well.

But data:text/html,<select></select> didn't trigger a crash.

Unfortunately, I don't have an en-us version of windows, the CD is only in french...

I didn't really understand what I should do with the msaa spy and how... ?
I found http://www.mozilla.org/access/windows/msaa-server but it didn't really help.

Comment 21

[timeless]

yeah, we'd almost certainly have to write one.

i think for now, it shouldn't be too hard to write code in accessibility which detects jhook/jaws 7.1 and just doesn't send this event.

Comment 22

[Aaron Leventhal]

Question -- is focusing the location bar enough to cause a crash? Or does the list need to open?

Is it just that the crash happens when an option gets focused in an opened list from a combobox? It sounds like it doesn't matter whether it is a XUL or HTML combo box.

Comment 23

[Aaron Leventhal]

This isn't happening in Firefox 2. Therefore we could simply test builds from the archives see what day it broke and look for what we changed in how we expose combobox/menulist/autocomplete, for both HTML and XUL.

I seem to recall doing quite a bit to those all at once. There may have been some changes that I made that seemed correct, but are actually unnecessary and could be removed if it fixes this.

Comment 24

[Aaron Leventhal]

There are suspicious fixes in the 2.0-3.0 timeframe that could have caused it. Here are some:

(These first two are the same day)
1.92	aaronleventhal%moonset.net	2007-11-15 11:53	 	Bug 403794. GetState() cleanup to fix some crashes on defunct objects and some incorrect states, and remove some code that is no longer necessary. r=surkov, a=dsicore
1.91	aaronleventhal%moonset.net	2007-11-15 11:46	 	Bug 391490. Every single <option> shows up in JAWS virtual cursor mode. Terrible for screen reader usability. r=ginn.chen, a=dsicore

1.85	ginn.chen%sun.com	2007-06-13 03:54	 	Bug 383355 State incorrectly set for showing item in a collapsed XUL menulist
r=aaronleventhal

1.84	aaronleventhal%moonset.net	2007-06-05 13:02	 	Bug 382270. The displayed menu item in a collapsed HTML combo box should have SHOWING state exposed via AT-SPI. r=evan.yan

1.81	aaronleventhal%moonset.net	2007-05-16 06:59	 	Bug 380669. More states for HTML option accessibles. r=surkov

1.80	aaronleventhal%moonset.net	2007-05-06 20:12	 	Bug 379678. Fix up HTML combo box accessibility. r=surkov

So the question is, when did the crashes begin?

Comment 25

[Marco Zehe (:MarcoZ)]

This broke between 2007040704 and 2007040804.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-07+03:00&maxdate=2007-04-08+05:00&cvsroot=/cvsroot

Comment 26

[Aaron Leventhal]

That's when we started again allowing QI to IA2. That's the fix for bug 376753 - Allow QueryInterface to IA2 witout crashing, patches=aaronlev, me, r=me, aaronlev 

I bet if you remove that from nsAccessibleWrap::QueryInterface() it probably stops crashing. If that's true it doesn't tell us much except that it's IA2 usage causing the crash.

But, the interesting thing is that we were re-enabling IA2 support and that we had it before. 

In fact all we did in that patch is uncomment the support for IAccessible2 in QueryInterface. We had actually enabled support at one point earlier:
1.60 <[e-mail]> 2007-03-17 22:21
Fix for bug 370790 - implement IAccessibleAction, r=aaronlev, ginn.chen

There may be a more interesting regression range earlier than when that checkin took affect on March 18, 2007.

Comment 27

[Marco Zehe (:MarcoZ)]

Yes, March 18, 2007, is also broken, March 17, 2007 is not.

Comment 28

[Marco Zehe (:MarcoZ)]

It should be noted that JAWS 7.10 doesn't come anywhere near IA2. It does not know anything about it. JAWS 8 was the first to ever implement IA2 support, for Symphony, and it got enabled late in the game I believe.

Comment 29

[Aaron Leventhal]

Marco, that might not be true. Simply comment out the lines in nsAccessibleWrap::QueryInterface() which allow IAccessible2 to be used and I believe the crash will go away. That would mean JAWS 7.1 is doing *something*, even if it is small, with IA2.

Comment 30

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Thanks to pointers by Timeless and mfinkle, and some help from slee, I got this one put together. If JAWS 7.10 is running, it turns off IA2 completely. It does the same for JAWS 8, but not if it's 8.0.2173. JAWS 9, NVDA, and Window-Eyes all have IA2 enabled. This eliminates the crash and actually allows surfing with JAWS 7.10 in the same fashion as 8 or 9.

Comment 31

[Marco Zehe (:MarcoZ)]

Comment on attachment 332330 [details] [diff] [review]
Patch

Better patch coming up. This one doesn't take into account versions of JAWS greater than 9.

Comment 32

[Marco Zehe (:MarcoZ)]

Attachment: Better patch

Now does not include JAWS 10 or any other higher version number from using IA2.

Comment 33

[Aaron Leventhal]

There are some variables like dwSecondLeft and dwRightMost that you don't use. It might be good to leave them in, but commented out, in case we need them later.

Nit: you have an unnecessary return; at the end of a method

I would prefer that gIsIA2CompatibleJawsLoaded becomes 
PRBool gIsIA2Disabled
which would default to PR_FALSE
Then you can do
  if (dwLeftMost < 8
      || (dwLeftMost == 8 && dwSecondRight < 2173))
    // All versions below 8.0.2173 are not compatible
    gIsIA2Disabled = PR_TRUE;
and you won't need an else, because that static var will be set to PR_FALSE by default.

I believe we are going to need version # for Window-Eyes as well at some point, because of the auto upgrade feature. I think we'll also need to avoid using the new Ctrl+Tab feature for all versions of JAWS and Window-Eyes for the time being.
	

Comment 34

[Aaron Leventhal]

Here's the big problem :)
QueryInterface() will no longer work for IAccessibleComponent, IAccessibleHyperlink or IAccessibleValue(), even for NVDA, Window-Eyes and later versions of JAWS. They just will never succeed.

Your IA2 disabling condition wrapped too much of ::QueryInterface() inside. You only want to know QI to IAccessible2.

Comment 35

[Marco Zehe (:MarcoZ)]

Attachment: Address Aaron's review comments

Comment 36

[Aaron Leventhal]

Comment on attachment 332398 [details] [diff] [review]
Address Aaron's review comments

IMO, the two-lined |if| in QueryInterface would be cleaner as a single line with && between the two conditions.

Also, the name DetermineJAWSVersion() doesn't indicate that it would change whether IA2 is disabled. You might consider changing the name to InitATSpecifics() or something. Then the method can be later used to change any special Window-Eyes settings as well, and it would be more clear that it *does* something.

Anyway, r+. You can get an sr= from Neil or Surkov if you want more advice.

Comment 37

[Marco Zehe (:MarcoZ)]

Attachment: Address more review comments

1. Turned the function into one that returns a PRBool.
2. Created another function, DoATSpecificProcessing, which currently only acts on the return value of IsOnlyMsaaCompatibleJawsPresent, but can be enhanced with other AT specific stuff later.
3. In nsAccessibleWrap::QueryInterface, put the two if checks into one statement, as requested by Aaron.

Comment 38

[alexander :surkov (:asurkov)]

Comment on attachment 332503 [details] [diff] [review]
Address more review comments

Ok, I'm not sure I'm happy with the approach to check version of screen readers. But can you say me why specific Jaws version has a crash when they aren't supposed to work with IA2? Why do they try the IAccessible2 interface if they do not work with it?

> /// the accessible library and cached methods
> HINSTANCE nsAccessNodeWrap::gmAccLib = nsnull;
> HINSTANCE nsAccessNodeWrap::gmUserLib = nsnull;
> LPFNACCESSIBLEOBJECTFROMWINDOW nsAccessNodeWrap::gmAccessibleObjectFromWindow = nsnull;
> LPFNNOTIFYWINEVENT nsAccessNodeWrap::gmNotifyWinEvent = nsnull;
> LPFNGETGUITHREADINFO nsAccessNodeWrap::gmGetGUIThreadInfo = nsnull;
> 
> PRBool nsAccessNodeWrap::gIsEnumVariantSupportDisabled = 0;
>+// Used to determine whether AN IAccessible2 compatible screen reader is loaded.

'AN' is capital letters, should be 'an' I guess.

> 
>+PRBool nsAccessNodeWrap::IsOnlyMsaaCompatibleJawsPresent()

could you put more empty lines into the code of this method, it will help to read the code.

>+
>+void nsAccessNodeWrap::DoATSpecificProcessing()

I think it doesn't make sense to keep this method because it's very small.

>+{
>+  if (IsOnlyMsaaCompatibleJawsPresent)

is it always true? Did you forget ()?

>+    // All versions below 8.0.2173 are not compatible
>+    gIsIA2Disabled  = PR_TRUE;
>+}

>-  else if (IID_IAccessible2 == iid)
>+  else if (IID_IAccessible2 == iid && !gIsIA2Disabled)

we shouldn't have similar check on another objects?

Comment 39

[Marco Zehe (:MarcoZ)]

(In reply to comment #38)
> Ok, I'm not sure I'm happy with the approach to check version of screen
> readers. But can you say me why specific Jaws version has a crash when they
> aren't supposed to work with IA2? Why do they try the IAccessible2 interface if
> they do not work with it?
I don't know why they crash. I also don't know why they query for IA2 in the first place. I believe 7.10 was the first to ever implement anything for IA2, even though they didn't use it properly yet back then.

Fact is that, if I disable the QI to IA2, Firefox does not crash with JAWS 7.10. That's why we decided to take this approach. It's hacky, I know, but we currently have no other choice. The module that crashes and takes Firefox down with it is JHook.dll, a portion of code that gets injected into Firefox by JAWS.

> >+void nsAccessNodeWrap::DoATSpecificProcessing()
> I think it doesn't make sense to keep this method because it's very small.

I did it for possible future extensibility. For example if we discover that it would be best to turn off IA2 support for Window-Eyes 5.5 as well or the like, we can easily implement a new method and simply add it to the check in this small method then.

> >+  if (IsOnlyMsaaCompatibleJawsPresent)

Arrggh, that should've been:

> >+  if (IsOnlyMsaaCompatibleJawsPresent())

Thanks!

> >+  else if (IID_IAccessible2 == iid && !gIsIA2Disabled)
> we shouldn't have similar check on another objects?

Didn't make any difference. With this check, JAWS 7.10 and Firefox no longer crash. So I decided to keep it simple.

Comment 40

[Marco Zehe (:MarcoZ)]

Attachment: Address Surkov's comments

Comment 41

[Marco Zehe (:MarcoZ)]

Comment on attachment 332683 [details] [diff] [review]
Address Surkov's comments

Cancelling SR request from Neil because he's on vacation. Looking for new super-reviewer.

Comment 42

[Marco Zehe (:MarcoZ)]

Comment on attachment 332683 [details] [diff] [review]
Address Surkov's comments

Brendan, can you take a look at this and see if you find any glaring issues? This patch prevents Firefox from replying with IAccessible2 capabilities once it detects that an incompatible version of the JAWS screen reader is running/injected as a module.

Comment 43

[Brendan Eich [:brendan]]

Comment on attachment 332683 [details] [diff] [review]
Address Surkov's comments

Looks ok to me, but I was just checking the disabling logic.

/be

Comment 44

[Marco Zehe (:MarcoZ)]

Pushed in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/cb95cbab6262

Comment 45

[timeless]

Comment on attachment 332683 [details] [diff] [review]
Address Surkov's comments

>+PRBool nsAccessNodeWrap::IsOnlyMsaaCompatibleJawsPresent()
>+  LPBYTE versionInfo = new BYTE[length];
you should null check this
>+  ::GetFileVersionInfoW(fileName, 0, length, versionInfo);
>+
>+  UINT uLen;
>+  VS_FIXEDFILEINFO *fixedFileInfo;

>@@ -153,21 +153,29 @@ class nsAccessNodeWrap :  public nsAcces
>     static int FilterA11yExceptions(unsigned int aCode, EXCEPTION_POINTERS *aExceptionInfo);
> 
>+    static PRBool IsOnlyMsaaCompatibleJawsPresent();
>+    static void DoATSpecificProcessing();

please consider inserting a blank line here

>   protected:

anyway, i'm very glad to see you got this working :)

Comment 46

[Marco Zehe (:MarcoZ)]

Hmm, should I just add these in a followup push, or file a separate bug on these?

Comment 47

[Marco Zehe (:MarcoZ)]

Attachment: Address Timeless' comments

Like this?

Comment 48

[Brendan Eich [:brendan]]

Timeless, we are moving away from new failure being a possibility (malloc too). Cc'ing bsmedberg. If this patch is for 1.9.0.x and older, sure. For 1.9.1 I'm not sure we need the null check.

Marco: new bug for followup patch is best, but let's sort out the new failure issue first. Thanks,

/be

Comment 49

[Xavier Robin]

Verified in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/20080809033929 Minefield/3.1a2pre 
Could surf without crash on several sites during several minutes.

Should this go to Firefox 3.0.x as well? It would be nice, especially if 3.1 is released after term of 2.0-series support.

Comment 50

[Marco Zehe (:MarcoZ)]

Comment on attachment 332683 [details] [diff] [review]
Address Surkov's comments

Requesting that this be taken for Firefox 3.0.2. It will allow more screen reader users to enjoy the benefits of 3.0.

Comment 51

[Marco Zehe (:MarcoZ)]

Comment on attachment 332951 [details] [diff] [review]
Address Timeless' comments

Timeless, could you review this one for inclusion into 1.9.0.2?

Comment 52

[Samuel Sidler (old account; do not CC)]

Not blocking, but wanted.

Marco, you have approval requested on attachment 332683 [details] [diff] [review]. Did you mean to request it on attachment 332951 [details] [diff] [review] which you had timeless review?

Comment 53

[Marco Zehe (:MarcoZ)]

Comment on attachment 332951 [details] [diff] [review]
Address Timeless' comments

I meant to ask for approval for this one as well, this is on top of the other patch.

Comment 54

[Samuel Sidler (old account; do not CC)]

Comment on attachment 332951 [details] [diff] [review]
Address Timeless' comments

Approved for 1.9.0.2. Please land in CVS. a=ss

Make sure both patches get in. Typically we like rollup patches on the branch, for future reference.

Comment 55

[Marco Zehe (:MarcoZ)]

Checking in accessible/build/Makefile.in;
/cvsroot/mozilla/accessible/build/Makefile.in,v  <--  Makefile.in
new revision: 1.41; previous revision: 1.40
done
Checking in accessible/src/msaa/nsAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp,v  <--  nsAccessibleWrap.cpp
new revision: 1.121; previous revision: 1.120
done
Checking in accessible/src/msaa/nsAccessNodeWrap.cpp;
/cvsroot/mozilla/accessible/src/msaa/nsAccessNodeWrap.cpp,v  <--  nsAccessNodeWrap.cpp
new revision: 1.45; previous revision: 1.44
done
Checking in accessible/src/msaa/nsAccessNodeWrap.h;
/cvsroot/mozilla/accessible/src/msaa/nsAccessNodeWrap.h,v  <--  nsAccessNodeWrap.h
new revision: 1.14; previous revision: 1.13
done

Comment 56

[Marco Zehe (:MarcoZ)]

Attachment: For reference, Patch that went into 1.9.0.2.

Comment 57

[Docteur Fas]

Verified in 
rv:1.9.0.2pre) Gecko/2008081705 
GranParadiso/3.0.2pre


It is OK.

Comment 58

[Marco Zehe (:MarcoZ)]

See comment # 57 for verification. Thanks!

Comment 59

[Marco Zehe (:MarcoZ)]

See comment #57 and comment #49 for verification.

Comment 60

[[e-mail]]

Comment on attachment 332683 [details] [diff] [review]
Address Surkov's comments

>+#include <tchar.h> 
Trailing space. Also strange that you would need to include this explicitly.

>+  HMODULE jhookhandle = ::GetModuleHandleW(NS_LITERAL_STRING("jhook").get());
You can use L"jhook" here (like you did for L"\\" below).