If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Mozilla crashes when magnifier is used

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: H. Stokhorst, Assigned: Aaron Leventhal)

Tracking

({access, crash})

Trunk
x86
Windows 2000
access, crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

15 years ago
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

15 years ago
can you run a Talkback-enabled Mozilla and send Talkback report ?
Severity: major → critical
Keywords: crash, stackwanted

Comment 2

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

Comment 3

15 years ago
WORKSFORME, moz1.1 on Win2k.
(Reporter)

Comment 4

15 years ago
Cannot reproduce this bug anymore on Mozilla 1.2

Comment 5

15 years ago
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

15 years ago
Reproduced with 2002110808-trunk/WinXP.
Talkback ID: TB13772371M
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

15 years ago
Whiteboard: TB13772371M

Comment 7

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

Comment 8

15 years ago
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

15 years ago
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)

Assignee: asa → law
Component: Browser-General → XP Toolkit/Widgets
Keywords: stackwanted → nsbeta1
QA Contact: asa → sairuh

Comment 10

15 years ago
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

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

Comment 12

15 years ago
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.
Keywords: access

Comment 13

15 years ago
.
Assignee: law → aaronl
Component: XP Toolkit/Widgets → Accessibility APIs
QA Contact: sairuh → dsirnapalli
Whiteboard: TB13772371M
(Assignee)

Comment 14

15 years ago
Created attachment 113739 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #113739 - Flags: superreview?(dougt)
Attachment #113739 - Flags: review?(jgaunt)

Comment 15

15 years ago
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.
(Assignee)

Comment 16

15 years ago
> 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

15 years ago
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.
Attachment #113739 - Flags: superreview?(dougt) → superreview?(dbaron)
(Assignee)

Comment 18

15 years ago
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'
(Assignee)

Comment 19

15 years ago
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?
(Assignee)

Updated

15 years ago
Attachment #113739 - Flags: review?(jgaunt) → review?(dougt)
(Assignee)

Comment 20

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

Updated

15 years ago
Attachment #113739 - Flags: review?(dougt) → review+
(Assignee)

Updated

15 years ago
Attachment #113739 - Flags: superreview?(dbaron) → superreview?(jst)

Comment 21

15 years ago
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

15 years ago
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.
(Assignee)

Comment 23

15 years ago
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

15 years ago
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 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
Attachment #113739 - Flags: superreview?(jst) → superreview+
(Assignee)

Updated

15 years ago
Attachment #113739 - Flags: approval1.3?
Attachment #113739 - Flags: approval1.3? → approval1.3+
(Assignee)

Comment 26

15 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 27

15 years ago
Still crashes.

TB17205010W
TB17534802Y
TB17534785Q
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 28

15 years ago
Created attachment 116776 [details]
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.
(Assignee)

Comment 29

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

Comment 30

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

Comment 31

15 years ago
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

15 years ago
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]
(Assignee)

Comment 33

15 years ago
Dbradley, that's probably fixed by bug 202972.

Comment 34

15 years ago
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

15 years ago
Created attachment 121443 [details] [diff] [review]
A potential fix, checks for null

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

Comment 36

15 years ago
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-
(Assignee)

Updated

15 years ago
Attachment #121443 - Flags: review+

Updated

15 years ago
Attachment #121443 - Flags: superreview?(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.
Attachment #121443 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 38

15 years ago
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

15 years ago
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

15 years ago
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);
}

(Assignee)

Comment 41

15 years ago
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

15 years ago
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.
(Assignee)

Comment 43

15 years ago
David, thanks.

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

Comment 44

15 years ago
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   }
(Assignee)

Comment 45

15 years ago
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.
(Assignee)

Comment 46

15 years ago
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.
(Assignee)

Comment 47

15 years ago
David, can I mark this fixed now?
(Assignee)

Comment 48

15 years ago
fixed
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.