287359 - Adding dump() loops in the History sidebar code causes crash [@ nsXULMenuitemAccessible::GetRole]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Gordon P. Hemsley [:GPHemsley]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050320
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050320

While adding dumps to try to get familiar with the Mozilla code in order to fix
another bug, I stumbled across something that repeatedly crashes. First it was
an accident, but I was able to reproduce it every time I tried--but only when I
was using my edited code. When I restored the original code, I was not able to
reproduce the crash. However, the files I had been editing were JavaScript files
(inside JARs), and I'm told by ajschult that "technically, you're not supposed
to be able to crash from javascript."

Reproducible: Always

Steps to Reproduce:
1. Make changes to files, and go about reimplementing the edited JAR files.
2. Load Mozilla with the command line option "-console" and press F9 to open the
sidebar.
3. In the History tab, quickly click on the various listings there. Eventually
the webpages will stop loading, the listings will stop changing highlighting,
and Mozilla will crash.

Actual Results:  
Mozilla crashed and needed to close all windows, including the loaded debugging
console.

Expected Results:  
The code should just keep dumping the things it was supposed to, via dump(),
just as it had when things were clicked slowly.

Two Talkback crash IDs available, though they contain mostly the same data:
TB4535094Z and TB4535354X.

I'm told by opi and ajschult that this is a regression caused by the resolution
of bug 257638.

Apparently, the crash happens in accessibility.dll.

Comment 1

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Code changes that caused crash

The crash was most likely caused by the code changed in the history files, but
I added the bookmark changes to the diff also because those changes were
implemented simultaneously (and were removed during the original code check).

Comment 2

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Code changes that caused crash

The crash was most likely caused by the code changed in the history files, but
I added the bookmark changes to the diff also because those changes were
implemented simultaneously (and were removed during the original code check).

(The first patch was not processed by Bugzilla correctly, as a diff for two
different files. I added a new line to this one to hopefully alleviate the
problem.)

Comment 3

[Andrew Schultz]

the code that's crashing was added in bug 257638

Comment 4

[Aaron Leventhal]

Louie, want to look at this?

Comment 5

[timeless]

Incident ID: 4535094
Stack Signature	nsXULMenuitemAccessible::GetRole c25f7037
Product ID	MozillaTrunk
Build ID	2005032005
Trigger Time	2005-03-22 18:30:38.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	accessibility.dll + (00011609)
URL visited	
User Comments	
Since Last Crash	30 sec
Total Uptime	30 sec
Trigger Reason	Access violation
Source File, Line No.
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/accessible/src/xul/nsXULMenuAccessible.cpp,
line 182
Stack Trace 	
nsXULMenuitemAccessible::GetRole 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/accessible/src/xul/nsXULMenuAccessible.cpp,
line 182]
nsDocAccessibleWrap::FireToolkitEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/accessible/src/msaa/nsDocAccessibleWrap.cpp,
line 189]
nsDocAccessible::FlushPendingEvents 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/accessible/src/base/nsDocAccessible.cpp,
line 993]
nsDocAccessible::FlushEventsCallback 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/accessible/src/base/nsDocAccessible.cpp,
line 1003]
nsAppStartup::Run 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/components/startup/src/nsAppStartup.cpp,
line 208]
main 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1817]
WinMain 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1841]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 6

[Justin Wood (:Callek)]

Aaron this is what I pinged you about, I'll fix this tonight for you ;-)

Comment 7

[Louie Zhao]

The crash may be caused by "'element' is NULL". If so, assertion "No DOM element
for menu node!" should be output. Till now, I am not clear why there is no DOM
element for the menu node. One possibility is that the DOM node for the menu
node has been destroyed before the corresponding accessible object. If this is
the reason, the crash can be caused by many places other than
nsXULMenuitemAccessible::GetRole. For example:
http://lxr.mozilla.org/seamonkey/source/accessible/src/xul/nsXULMenuAccessible.cpp#67
http://lxr.mozilla.org/seamonkey/source/accessible/src/xul/nsXULMenuAccessible.cpp#107

Comment 8

[Aaron Leventhal]

Louie, no assertion is necessary. It can happen at any time that mDOMNode for an
accessible is null.

A node may go away in a document, in which case we call Shutdown() on the
accessible for it, which set mDOMNode to null. That way if any external process
is refcounting the accessible object they end up holding onto a stub that does
not refcount anything else. If they call any methods on that dead object they
won't crash, but should get an error returned.

Comment 9

[Justin Wood (:Callek)]

Attachment: Bail on null (Checked in)

Assertion removed from talk with aaron, Other areas where mDOMNode being null
could cause crash untouched.

Mainly due to me not knowing the code involved, so no clue what to set the out
value's to.

Comment 10

[Aaron Leventhal]

Comment on attachment 178429 [details] [diff] [review]
Bail on null (Checked in)

That's exactly right. We shouldn't assert, we should return NS_ERROR_FAILURE.
This is actually a very normal occurance as the assistive technology can still
be asking about something it's holding onto after ther DOM node has gone away.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 178429 [details] [diff] [review]
Bail on null (Checked in)

sr=dmose

Comment 12

[Ian Neal]

Comment on attachment 178429 [details] [diff] [review]
Bail on null (Checked in)

Checking in nsXULMenuAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULMenuAccessible.cpp,v  <-- 
nsXULMenuAccessible.cpp
new revision: 1.33; previous revision: 1.32
done

Comment 13

[Stefan [:stefanh]]

--> Fixed