166504 - Mozilla crashes when magnifier is used

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[H. Stokhorst]

Windows 2000 Version 5.0 Build 2195
Mozilla 1.1 (Gecko/20020826)
Steps to reproduce:

0 Start Windows 2000
1 Start Mozilla
2 Close dial up screen (connect)
3 Close dialog "www.myhompage.tld could not be found" (alert)
4 Close Mozilla
- (nothing happens)
5 Start | Programs | Accessoires | Accessibility | Magnifier
6 As 1
7 As 2
8 As 3
9 (Move Mozilla window so the titlebar can be reached)
10 As 4
- The instruction at "0x6061e482" referenced memory at "0x001156688" The memory
could not be read. Click OK to terminate

Reproducable: always

Comment 1

[Olivier Cahagne]

can you run a Talkback-enabled Mozilla and send Talkback report ?

Comment 2

[Frederic Bezies]

Well, cannot reproduce this bug, using WinXP and trunk build 2002090308.

Comment 3

[Benjamin Smedberg]

WORKSFORME, moz1.1 on Win2k.

Comment 4

[H. Stokhorst]

Cannot reproduce this bug anymore on Mozilla 1.2

Comment 5

[Wesley Moy]

Confirm Bug: Windows XP Professional, Mozilla 1.2b 2002110808
(someone please send me E-mail teaching how to get TalkBack ID)

When Magnifier is open and Mozilla is busy (ie any time throbber is active) when
closing mozilla, said error message occurs.

Comment 6

[Koike Kazuhiko]

Reproduced with 2002110808-trunk/WinXP.
Talkback ID: TB13772371M

Comment 7

[alanjstr]

*** Bug 181663 has been marked as a duplicate of this bug. ***

Comment 8

[alanjstr]

TB14305116x submitted for Mozilla/5.0 (Windows; U; Windows NT 5.0;
en-US; rv:1.0.1) Gecko/20020826.

Also crashes latest nightly and 11/19 Phoenix build.

I think the other Magnify bugs are listed as XP Toolkit/Widgets

Comment 9

[John Morrison]

Seems like it's DDE related. I can reproduce as noted in comment #5 above on
win2k with a current trunk build when magnify.exe is running.

-> law and nsbeta1 for consideration

Stack trace for talkback 13772371 is as follows, although that may 
be a red herring. The talkback stack for TB14305116x in the duplicate
bug was empty. I can't get a stack trace from my opt-with-symbols 
build, but I can crash each time on exit.

SearchTable [d:/builds/seamonkey/mozilla/xpcom/ds/pldhash.c, line 359]
PL_DHashTableOperate [d:/builds/seamonkey/mozilla/xpcom/ds/pldhash.c, line 551]
AtomImpl::~AtomImpl [d:/builds/seamonkey/mozilla/xpcom/ds/nsAtomTable.cpp, line 
179]
AtomImpl::`scalar deleting destructor'
nsPipe::QueryInterface [d:/builds/seamonkey/mozilla/xpcom/io/nsPipe2.cpp, line 
220]
nsHTMLAttrName::Release 
[d:/builds/seamonkey/mozilla/content/html/style/src/nsHTMLAttributes.h, line 
181]
nsHTMLAttributes::Reset 
[d:/builds/seamonkey/mozilla/content/html/style/src/nsHTMLAttributes.cpp, line 
1545]
gkwidget.dll + 0x0 (0x61490000)
ntdll.dll + 0x2572a (0x77f7572a)
ntdll.dll + 0xe52d (0x77f5e52d)
kernel32.dll + 0x198cc (0x77e398cc)
kernel32.dll + 0x1990f (0x77e3990f)
msvcrt.dll + 0x279c8 (0x77be79c8)
kernel32.dll + 0x214c7 (0x77e414c7)

Comment 10

[alanjstr]

How about TB14333795M or TB14333772K from Mozilla/5.0 (Windows; U; Windows NT
5.0; en-US; rv:1.1) Gecko/20020826

I can crash a 1.3alpha if need be.

Comment 11

[Olivier Cahagne]

a crash with Mozilla 1.3a latest nightly build is preferred as code has changed
a lot since 1.1.

Comment 12

[John Morrison]

Both 14333795 and 14333772 have this crash stack:

0x01e4732d
RootAccessible::`scalar deleting destructor'
SimpleDOMNode::Release 
[c:/builds/seamonkey/mozilla/widget/src/windows/SimpleDOMNode.cpp, line 147]
ole32.dll + 0x32b94 (0x77a82b94)
ole32.dll + 0x5f500 (0x77aaf500)
ole32.dll + 0x305a4 (0x77a805a4)
ole32.dll + 0x30301 (0x77a80301)
ole32.dll + 0x30001 (0x77a80001)
0xffffffff 

But a more recent build might be better.

Comment 13

[timeless]

.

Comment 14

[Aaron Leventhal]

Attachment: Part 1 of fix, fixes a lot of MSAA shutdown problems

It turns out that an MSAA client can release stuff when it's very inconvenient
for us. We need shutdown methods to clear all of our references to internal
objects, while it's still okay to do so.

I want to save problems that are related to document objects for the patch I'm
working on for bug 180415.

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 113739 [details] [diff] [review]
Part 1 of fix, fixes a lot of MSAA shutdown problems

i am conserned about the Accessible object declaring its own Add/Release. 
These two symbols are exactly the same as the ones defined by the
SimplyDOMNode.	I don't think that you will actually need them. - you might
want to send dbradley an email, he is familar with doing stuff like this.

Comment 16

[Aaron Leventhal]

> i am conserned about the Accessible object declaring its own Add/Release. 
> These two symbols are exactly the same as the ones defined by the
> SimplyDOMNode. I don't think that you will actually need them. - you might
> want to send dbradley an email, he is familar with doing stuff like this.

I don't understand the concern. 

STDMETHODIMP_(ULONG) Accessible::AddRef()
NS_IMETHODIMP_(nsrefcnt) DocAccessible::AddRef(void)

One is for xpcom, one for com. If we don't have them both, it won't link.
However, they both do the exact same thing.

Comment 17

[timeless]

Comment on attachment 113739 [details] [diff] [review]
Part 1 of fix, fixes a lot of MSAA shutdown problems

technically dougt isn't an sr. and there doesn't appear to be any
mozilla/xpcom/<files> so it's not like you're asking for his moa.

Comment 18

[Aaron Leventhal]

If I try to use only XPCOM-style AddRef on SimpleDOMNode class, and remove the
declarations and definitions of AddRef from the derived classes, I get compile
errors:
c:/moz/mozilla/widget/src/windows/nsWindow.cpp(919) : error C2385:
'RootAccessible::Release' is ambiguous
c:/moz/mozilla/widget/src/windows/nsWindow.cpp(919) : warning C4385: could be
the 'Release' in base 'SimpleDOMNode' of base 'Accessible' of base 'DocAccessible'
 of class 'RootAccessible'
c:/moz/mozilla/widget/src/windows/nsWindow.cpp(919) : warning C4385: or the
'Release' in base 'IAccessible' of base 'Accessible' of base 'DocAccessible' of clas
s 'RootAccessible'
c:/moz/mozilla/widget/src/windows/nsWindow.cpp(919) : warning C4385: or the
'Release' in base 'IUnknown' of base 'IEnumVARIANT' of base 'Accessible' of base 'Do
cAccessible' of class 'RootAccessible'
c:/moz/mozilla/widget/src/windows/nsWindow.cpp(919) : warning C4385: or the
'Release' in base 'IUnknown' of base 'ISimpleDOMDocument' of base 'DocAccessible' of
 class 'RootAccessible'
c:/moz/mozilla/widget/src/windows/nsWindow.cpp(919) : warning C4385: or the
'Release' in base 'nsISupports' of base 'nsIAccessibleEventListener' of class 'RootA
ccessible'

Comment 19

[Aaron Leventhal]

I was able to change all of the AddRef/Release's to NS_IMETHODIMP_(nsrefcnt),
but not reduce the number of declarations/impl's of it and still compile + link.

However, it now crashes on startup.

That part of the code didn't change with this patch, and has nothing to do with
the crash. How about, if it works, don't fix it?

Comment 20

[Aaron Leventhal]

I've changed one thing in the patch, switching the order of 2 lines
mRootAccessible->Shutdown();
mRootAccessible->Release();
is the correct order.

Comment 21

[John Gaunt (redfive)]

Comment on attachment 113739 [details] [diff] [review]
Part 1 of fix, fixes a lot of MSAA shutdown problems

r=jgaunt

nit: you have a variable named 'r'. Your argument about index variables called
'i' applies to this too.

Comment 22

[David Bradley]

As far as you're patch goes, I doubt it's any worse off than the existing code.
I knew this was reminding me of something familiar, but it just took a little
time for it fall out of my brain. What Adam Lock and I did in similar situations
is to use tearoffs. This kept the COM interfaces and XPCOM interfaces separated.
I wonder if this code would be better served if refactored to use tearoffs. I
don't know enough about the entire structure and usage to say for sure, but
thought I'd throw it out.

The QI's really make me queezy. Consider that  QI to IUnknown is going to return
in returin nsISupports. This really probably isn't a big deal, but add that to
the multiple implementations of AddRef, that look like they appear in differnet
spots in the vtable, just makes me shiver.

That said, I'm sure refactoring this is no small task. This isn't portable code,
right? So if it's tested and it works, and this fixes a crash it's probably good
enough for now. Though I'd probably put looking into using a tearoff high on the
things to do for this code.

Comment 23

[Aaron Leventhal]

I don't think the two kinds of QI have the same signature. One gets called by
COM, the other by XPCOM. I had to implement both of them, so I don't think
there's a conflict actually.

// XPCOM QueryInterface
NS_IMPL_QUERY_INTERFACE1(RootAccessible, nsIAccessibleEventListener)

// Microsoft COM QueryInterface
STDMETHODIMP RootAccessible::QueryInterface(REFIID iid, void** ppv)


They each get used at different times, so I don't see the problem.

Comment 24

[David Bradley]

I think the situation with the QI is ok, so long as COM never calls the XPCOM
one and visa versa. I doesn't appear that can happen. As far as the
AddRef/Release I think if you get the compiler happy then you should be fine.
Since we're getting Addref/Release definitions coming in from other branches of
multiple inheritance you'll have to implement forwarders as you've done. One
thing to consider is to declare AddRef/Release inline, so that the functions
forwarding to these might be able to inline their calls and not incur an actual
function call.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 113739 [details] [diff] [review]
Part 1 of fix, fixes a lot of MSAA shutdown problems

- In RootAccessible::Shutdown():

+    if (--gActiveRootAccessibles == 0)   // Last root accessible. 
+      ShutdownAll();
+
+    gListCount = 0;

Seems to me that it would make more sense to move the setting of gListCount to
zero into ShutdownAll() which is where the loop that uses this variable to
clear out an array now lives.

sr=jst

Comment 26

[Aaron Leventhal]

checked in

Comment 27

[Koike Kazuhiko]

Still crashes.

TB17205010W
TB17534802Y
TB17534785Q

Comment 28

[David Bradley]

Attachment: Stacks from talkbacks listed

Not sure how helpful these will be. The first one looks like the heap was
trashed, ebp==0. The other two were identical.

The first one looked like it try to execute code in a DLL that had been
unloaded. The other two had jumped to 0x0 after jumping to what looks like a
bogus address.

Comment 29

[Aaron Leventhal]

I'll be doing more work on these crashers during this week.  Thanks David. 
I have a general understanding of the problem.

Comment 30

[Koike Kazuhiko]

WFM: 2003042208-trunk/Win2k doesn't crash.

Comment 31

[David Bradley]

Took a little bit, but it crashed on me. Not sure, but looks like a timer fired
on a dead object.

[E] IPR: Invalid pointer read in nsDocAccessible::FireDocLoadFinished(void) {1
occurrence}
        Reading 4 bytes from 0x0dc4dbd0 (4 bytes at 0x0dc4dbd0 illegal)
        Address 0x0dc4dbd0 points into a HeapAlloc'd block in unallocated region
of heap 0x003e0000
        Thread ID: 0xacc
        Error location
            nsDocAccessible::FireDocLoadFinished(void) [nsDocAccessible.cpp:386]
                void nsDocAccessible::FireDocLoadFinished()
                {
                  // Hook up our new accessible with our parent
             =>   if (!mParent) {
                    nsCOMPtr<nsIDocument> parentDoc;
                    mDocument->GetParentDocument(getter_AddRefs(parentDoc));
                    if (parentDoc) {
            nsTimerImpl::Fire(void) [nsTimerImpl.cpp:382]
            nsTimerManager::FireNextIdleTimer(void) [nsTimerImpl.cpp:614]
            nsAppShell::Run(void) [nsAppShell.cpp:152]
            nsAppShellService::Run(void) [nsAppShellService.cpp:477]
            main1          [nsAppRunner.cpp:1268]
            main           [nsAppRunner.cpp:1647]
            WinMain        [nsAppRunner.cpp:1671]
            ???            [ip=0x003e41b8]
            WinMainCRTStartup [crtexew.obj]

Comment 32

[David Bradley]

I missed this error, it occured before the one I just posted. Looks like the QI
might have failed, then the DCOM system caught the exception and it was lost.

[E] NPR: NULL pointer read in nsXULButtonAccessible::GetAccState(UINT *) {1
occurrence}
        Reading 4 bytes from 0x00000000 (4 bytes at 0x00000000 illegal)
        Address 0x00000000 points into invalid memory 
        Thread ID: 0xacc
        Error location
            nsXULButtonAccessible::GetAccState(UINT *)
[nsXULFormControlAccessible.cpp:150]
                  nsCOMPtr<nsIDOMElement> element(do_QueryInterface(mDOMNode));
                  NS_ASSERTION(element, "No nsIDOMElement for button node!");
                  PRBool isDefault = PR_FALSE;
             =>   element->HasAttribute(NS_LITERAL_STRING("default"), &isDefault) ;
                  if (isDefault)
                    *_retval |= STATE_DEFAULT;
                
            nsAccessibleWrap::get_accState(tagVARIANT,tagVARIANT *)
[nsAccessibleWrap.cpp:318]
             [RPCRT4.dll ip=0x78054190]
             [RPCRT4.dll ip=0x780773cb]
             [RPCRT4.dll ip=0x78072866]
             [OLEAUT32.dll ip=0x035c78fc]
             [OLE32.dll ip=0x772b5bc1]
             [OLE32.dll ip=0x771df4e8]

Comment 33

[Aaron Leventhal]

Dbradley, that's probably fixed by bug 202972.

Comment 34

[David Bradley]

It may be related/similar but it's not the same bug. The proposed fix in 202972
will not address the failed QI in nsXULButtonAccessible::GetAccState or the
invalid pointer in nsDocAccessible::FireDocLoadFinished. Bug 202972's patch
addresses a null mDocument pointer. I'm hoping that if the failed QI is
addressed in nsXULButtonAccessible::GetAccState that the follow up errors will
go away as well. Addressing this might actually solve the problem in bug 202972,
but I don't know for sure.

Comment 35

[David Bradley]

Attachment: A potential fix, checks for null

I ran with this and couldn't reproduce the errors I was getting before

Comment 36

[Samir Gehani]

adt: nsbeta1-

Comment 37

[Johnny Stenback (:jst)]

Comment on attachment 121443 [details] [diff] [review]
A potential fix, checks for null

Do we know why this happens? Is it mDOMNode that's null, or is mDOMNode not an
element? If the latter, then tere seems to be a bigger problem here.

sr=jst, but please move the declaration of isDefault after the if (!element)
check and early return.

Comment 38

[Aaron Leventhal]

My fix for bug 166504 and other recent accessibility topcrash fixes take care of
this in a way that addresses the root problem. The problem is that mDOMNode is
null, because the accessible node has been shutdown. This is a valid condition,
which is now checked for in in the get_accState() and other getters, before this
is even called. This null check is not really necessary, because a button will
always have a dom element.

Dbradley, if you check you should see that the specific crashes your patch fixes
don't happen anymore.

Let's mark it fixed once we verify that this is the case, or should we mark it
fixed now, and have you verify it?

Comment 39

[David Bradley]

I'm pretty sure in my case the mDOMNode wasn't null. The QI returns a null
pointer because what ever was in mDOMNode wasn't an nsIDOMElement. I'll double
check with a current build and see what happens.

Comment 40

[David Bradley]

Well, now I'm crashing in a different location. aWeakShell is valid, the QI of
it returns a null pointer and then crashes in the call. I'll see if I can
reproduce the original crash.

NS_IMETHODIMP nsAccessibilityService::GetAccessibleInWeakShell(nsIDOMNode *aNode, 
                                                               nsIWeakReference
*aWeakShell,
                                                               nsIAccessible
**aAccessible) 
{
  nsCOMPtr<nsIPresShell> presShell(do_QueryReferent(aWeakShell));
  return GetAccessible(aNode, presShell, aWeakShell, aAccessible);
}

Comment 41

[Aaron Leventhal]

David, with this new crash I don't think you have the fix for bug 204934. I
checked that in very early this morning.

Comment 42

[David Bradley]

I'm still getting crashes, although not the same and it's much more difficult to
get it to crash.

I'm pretty sure in my original crash the mDOMNode wasn't null, else I think it
would have crashed in the QI instead of the use of element. I can't tell if this
is truly fixed, or just hidden.

In any case, I'll see if I can get some info worth reporting from the crashes
I'm now seeing.

Comment 43

[Aaron Leventhal]

David, thanks.

BTW, QI doesn't crash when QI'ing from null, it returns null in that case.

Comment 44

[David Bradley]

I no longer can reproduce the original crash.

I do, however, occasionally get this crash on a null pointer on line 273. I
think the addition of !mParent created this condition. If mParent is null, I
don't think you want to enter this 'if' if mNextSibling is null as well, which
is what the current logic is doing.

268   if (mNextSibling || !mParent) {
269     // If no parent, don't try to calculate a new sibling
270     // It either means we're at the root or shutting down the parent
271     if (mNextSibling != DEAD_END_ACCESSIBLE) {
272       *aAccNextSibling = mNextSibling;
273       NS_ADDREF(*aAccNextSibling);
274     }
275     return NS_OK;
276   }

Comment 45

[Aaron Leventhal]

You're right. NS_IF_ADDREF is what I want there. That's bug 206344, which seems
to be the last big crash problem currently showing up with the new accessibiltiy
rewrite.

Comment 46

[Aaron Leventhal]

David, I just checked in bug 206344, which should fix the crash in
GetNextAccessible().

Hopefully that will allow us to close this bug as well.

Comment 47

[Aaron Leventhal]

David, can I mark this fixed now?

Comment 48

[Aaron Leventhal]

fixed