Closed
Bug 423176
Opened 16 years ago
Closed 16 years ago
Crash with jaws 7.1 [@ jhook.dll@0x5311]
Categories
(Firefox :: Disability Access, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: bugmail-mozilla, Assigned: MarcoZ)
References
()
Details
(Keywords: access, crash, verified1.9.0.2)
Crash Data
Attachments
(4 files, 3 obsolete files)
5.42 KB,
patch
|
aaronlev
:
review+
|
Details | Diff | Splinter Review |
5.61 KB,
patch
|
brendan
:
superreview+
samuel.sidler+old
:
approval1.9.0.2+
|
Details | Diff | Splinter Review |
1.66 KB,
patch
|
timeless
:
review+
samuel.sidler+old
:
approval1.9.0.2+
|
Details | Diff | Splinter Review |
6.08 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•16 years ago
|
Component: General → Disability Access
QA Contact: general → disability.access
Reporter | ||
Comment 1•16 years ago
|
||
Might be related to bug 420960, but it doesn't crash on the given examples at dojotoolkit.org.
Comment 2•16 years ago
|
||
Xavier, does breakpad not appear? Having a crash stack would certainly help.
Reporter | ||
Comment 3•16 years ago
|
||
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
Summary: Crash with jaws → Crash with jaws [@ jhook.dll@0x5311]
Comment 4•16 years ago
|
||
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•16 years ago
|
||
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/
Assignee | ||
Comment 6•16 years ago
|
||
Also, what JAWS version were you using?
Reporter | ||
Comment 7•16 years ago
|
||
(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.
Assignee | ||
Comment 8•16 years ago
|
||
(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.
Assignee | ||
Comment 9•16 years ago
|
||
In addition, I cannot reproduce the problem with JAWS 8.
Reporter | ||
Comment 10•16 years ago
|
||
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...
Assignee | ||
Comment 11•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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?
Assignee | ||
Comment 14•16 years ago
|
||
(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•16 years ago
|
||
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•16 years ago
|
||
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.
Summary: Crash with jaws [@ jhook.dll@0x5311] → Crash with jaws 7.1 [@ jhook.dll@0x5311]
Reporter | ||
Comment 17•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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.
Reporter | ||
Comment 20•16 years ago
|
||
> 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•16 years ago
|
||
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•16 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 23•16 years ago
|
||
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•16 years ago
|
||
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?
Assignee | ||
Comment 25•16 years ago
|
||
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•16 years ago
|
||
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 <surkov.alexander@gmail.com> 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.
Assignee | ||
Comment 27•16 years ago
|
||
Yes, March 18, 2007, is also broken, March 17, 2007 is not.
Assignee | ||
Comment 28•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Comment 30•16 years ago
|
||
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.
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
Attachment #332330 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 31•16 years ago
|
||
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.
Attachment #332330 -
Attachment is obsolete: true
Attachment #332330 -
Flags: review?(aaronleventhal)
Assignee | ||
Comment 32•16 years ago
|
||
Now does not include JAWS 10 or any other higher version number from using IA2.
Attachment #332332 -
Flags: review?(aaronleventhal)
Comment 33•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Comment 35•16 years ago
|
||
Attachment #332332 -
Attachment is obsolete: true
Attachment #332398 -
Flags: review?(aaronleventhal)
Attachment #332332 -
Flags: review?(aaronleventhal)
Comment 36•16 years ago
|
||
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.
Attachment #332398 -
Flags: review?(aaronleventhal) → review+
Assignee | ||
Comment 37•16 years ago
|
||
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.
Attachment #332503 -
Flags: superreview?(neil)
Comment 38•16 years ago
|
||
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?
Assignee | ||
Comment 39•16 years ago
|
||
(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.
Assignee | ||
Comment 40•16 years ago
|
||
Attachment #332503 -
Attachment is obsolete: true
Attachment #332683 -
Flags: superreview?(neil)
Attachment #332503 -
Flags: superreview?(neil)
Assignee | ||
Comment 41•16 years ago
|
||
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.
Attachment #332683 -
Flags: superreview?(neil)
Assignee | ||
Comment 42•16 years ago
|
||
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.
Attachment #332683 -
Flags: superreview?(brendan)
Comment 43•16 years ago
|
||
Comment on attachment 332683 [details] [diff] [review] Address Surkov's comments Looks ok to me, but I was just checking the disabling logic. /be
Attachment #332683 -
Flags: superreview?(brendan) → superreview+
Assignee | ||
Comment 44•16 years ago
|
||
Pushed in changeset: http://hg.mozilla.org/mozilla-central/index.cgi/rev/cb95cbab6262
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 45•16 years ago
|
||
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 :)
Assignee | ||
Comment 46•16 years ago
|
||
Hmm, should I just add these in a followup push, or file a separate bug on these?
Comment 48•16 years ago
|
||
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
Reporter | ||
Comment 49•16 years ago
|
||
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.
Flags: blocking1.9.0.2?
Assignee | ||
Comment 50•16 years ago
|
||
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.
Attachment #332683 -
Flags: approval1.9.0.2?
Assignee | ||
Comment 51•16 years ago
|
||
Comment on attachment 332951 [details] [diff] [review] Address Timeless' comments Timeless, could you review this one for inclusion into 1.9.0.2?
Attachment #332951 -
Flags: review?(timeless) → review+
Comment 52•16 years ago
|
||
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?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2-
Assignee | ||
Comment 53•16 years ago
|
||
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.
Attachment #332951 -
Flags: approval1.9.0.2?
Updated•16 years ago
|
Attachment #332683 -
Flags: approval1.9.0.2? → approval1.9.0.2+
Comment 54•16 years ago
|
||
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.
Attachment #332951 -
Flags: approval1.9.0.2? → approval1.9.0.2+
Assignee | ||
Comment 55•16 years ago
|
||
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
Keywords: access,
fixed1.9.0.2
Assignee | ||
Comment 56•16 years ago
|
||
Comment 57•16 years ago
|
||
Verified in rv:1.9.0.2pre) Gecko/2008081705 GranParadiso/3.0.2pre It is OK.
Assignee | ||
Comment 58•16 years ago
|
||
See comment # 57 for verification. Thanks!
Keywords: verified1.9.0.2
Updated•16 years ago
|
Keywords: fixed1.9.0.2
Assignee | ||
Comment 59•16 years ago
|
||
See comment #57 and comment #49 for verification.
Status: RESOLVED → VERIFIED
Comment 60•16 years ago
|
||
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).
Updated•13 years ago
|
Crash Signature: [@ jhook.dll@0x5311]
You need to log in
before you can comment on or make changes to this bug.
Description
•