235217 - Browser crashes on tree updates reporting error in ntdll.dll or js3250.dll

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Wladimir Palant]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040211

My sidebar extension crashes every now and then in Mozilla 1.6 or 1.7a, without
any sign of problems in Mozilla 1.5. I've managed to create a testcase, one XUL
file that contains a tree that is bound to an RDF datasource. The Rebuild button
empties the tree and refills it (this sometimes causes Mozilla to crash). I'm
not sure if this is the same bug as with the extension but at least it is
reproducible. The testcase usually produces errors in ntdll.dll, my extension
usually shows an error in js3250.dll. The addresses reported are always
different. Here is an example:

Problem Signature
AppName: mozilla.exe	 AppVer: 1.7.20040.21109	 ModName: ntdll.dll
ModVer: 5.1.2600.1106	 Offset: 00001d24

Exception Information
Code: 0xc0000005        Flags: 0x00000000
Record: 0x0000000000000000      Address: 0x0000000077f41d24

I couldn't create a talkback report, the talkback agent didn't install with the
latest Mozilla versions for some reason.

Reproducible: Always
Steps to Reproduce:
1. Download the testcase
2. Open it in the browser from file://
3. Accept any permission requests and click the Rebuild button until the browser
crashes (up to 10 times)

Comment 1

[Wladimir Palant]

Attachment: Testcase

Comment 2

[Hermann Schwab]

TalkbackID: TB30590581H
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219

http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/experimental/2004-02-19-14-1.7a/mozilla-win32-1.7a-installer.exe

Comment 3

[Wladimir Palant]

Attachment: Simplified testcase (some unnecessary details removed)

Comment 4

[Wladimir Palant]

The testcase works only with the dont-build-content flag.
There is a strange dependency on the number of tree rows. The crash occurs only
if the tree has 9 to 64 rows (2^3 < x <= 2^6). The browser doesn't crash if the
tree has 8 rows and it doesn't crash with 65 rows. First I thought it would
depend on the scrolling but it doesn't - changing window size or tree height
doesn't change anything. It looks more like the memory for some stack array
isn't reserved properly.

Comment 5

[Wladimir Palant]

I have done some bug searching in nsAutoVoidArray and nsSupportsArray -
http://bugzilla.mozilla.org/show_bug.cgi?id=235307#c2 could be the reason for
this bug (unfortunately I cannot build Mozilla to test it).

Comment 6

[Wladimir Palant]

Bug 235307 has been fixed, but this problem is still there in Build 2004022608 -
something else must be the problem.

Comment 7

[Denis Ibaev]

Mozilla/5.0 (Windows; U; Windows NT 5.0; ru-RU; rv:1.6) Gecko/20040113
MultiZilla/1.6.2.1a

Comment 8

[Wladimir Palant]

The bug doesn't occur with the debug build, only the optimized build crashes.
However, the optimized build doesn't crash in gdb. This makes it difficult to
trace...

Comment 9

[Wladimir Palant]

That's the stack trace for the crash on Win98:

HeapFree (KERNEL32.DLL)
delete (MSVC.DLL)
nsTreeRows::Clear(void)
nsXULTreeBuilder::RebuildAll(void)
nsXULTemplateBuilder::Rebuild(void)
XPC3250.DLL
XPC3250.DLL
JS3250.DLL
...

Comment 10

[Wladimir Palant]

I could finally identify this bug as a regression from bug 161452. Removing the
patch to nsXULTreeBuilder.cpp solves the problem. ccing neil.

Comment 11

[[e-mail]]

Jan, do I need a null check somewhere?

Comment 12

[Wladimir Palant]

Did some tests, the following change for
http://lxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULTreeBuilder.cpp#1115
should solve this bug:

-             if (mRows.RemoveRowAt(iter) == 0) {
+             if (mRows.RemoveRowAt(iter) == 0 && iter->mContainerType !=
nsTreeRows::eContainerType_Noncontainer) {

It doesn't break the patch to bug 161452 but makes sure we don't invalidate the
container when removing the last row from the tree. Neil, what do you think?

Comment 13

[Jan Varga [:janv]]

Where does it crash exactly?

Comment 14

[Wladimir Palant]

It's always a different stack trace... Some examples:

ChildEBP RetAddr  Args to Child              
0012e750 77f41dc9 00280000 01f7ad98 0012e7f0 ntdll!RtlpUnWaitCriticalSection+0x126
0012e824 77bfab2e 00280000 00000000 01f7ada0 ntdll!RtlpUnWaitCriticalSection+0x1cb
0012e86c 010b535d 01f7ada0 0109eb98 01f7ada0 msvcrt!free+0xc3
0012e874 0109eb98 01f7ada0 01f25aa0 0109eb34 gklayout!operator delete+0x9 (FPO:
[1,0,0])
0012e898 1003385a 02140068 00bff1b8 0012ea1c
gklayout!nsTreeRows__Subtree__Clear+0x3f (FPO: [EBP 0x02140068] [0,0,4])
0012e8a8 011d29ad 01978210 02140068 0012ea54
xpcom!xptiInterfaceInfo__GetIIDForParamNoAlloc+0x23 (FPO: [4,0,0])
02140068 01000000 00010000 00000000 00000000
xpc3250!XPCWrappedNative__CallMethod+0xac6
01010101 09755c3c 753bf303 eb2e730c 4dbe0f0b
gklayout!nsHTMLBodyElement__GetAttributeMappingFunction+0x74
3e048a37 00000000 00000000 00000000 00000000 0x9755c3c

ChildEBP RetAddr  Args to Child              
0012e87c 77f41dc9 00270000 01f98cc8 0012e91c ntdll!RtlpUnWaitCriticalSection+0x126
0012e950 77bfab2e 00270000 00000000 01f98cd0 ntdll!RtlpUnWaitCriticalSection+0x1cb
0012e998 61d6ff32 01f98cd0 61d701f3 01ef5e0c msvcrt!free+0xc3
0012e9a0 61d701f3 01ef5e0c 01f98cd0 01ef5df8 xpcom!PL_DHashFreeTable+0xa (FPO:
[2,0,0])
0012e9bc 6158cc06 01ef5e0c 01ef5e0c 6158c4b1 xpcom!PL_DHashTableFinish+0x47
(FPO: [EBP 0x0012e9e8] [1,0,4])
0012e9c8 6158c4b1 6163399c 00000000 01ef5e0c gklayout+0x16cc06 (FPO: [0,0,1])
0012e9e8 6158bda3 01ef8948 01ef5df8 60fc14f1 gklayout+0x16c4b1
0012e9f4 60fc14f1 01ef8948 01ef5df8 00000001 gklayout+0x16bda3 (FPO: [3,0,0])
0012ea18 6158bec3 00000007 01ef893c 6158c433 plds4!PL_HashTableDestroy+0x41
(FPO: [EBP 0x00000000] [1,1,4])
0012ea24 6158c433 01ef8898 6157ed94 01ef8898 gklayout+0x16bec3 (FPO: [0,0,1])
0012ea2c 6157ed94 01ef8898 01ef88b8 01ebc750 gklayout+0x16c433 (FPO: [0,0,1])
0012ea44 61586d43 0012ec20 01ef897c 0012ea64 gklayout+0x15ed94
0012ea58 61d976fa 01ef8898 0012ec04 610b2a31 gklayout+0x166d43 (FPO: [EBP
0x0012ea64] [1,0,4])
0012ea64 610b2a31 01ef8898 00000005 00000000 xpcom!XPTC_InvokeByIndex+0x27
0012ec04 610b5f0f 00000000 0198cd20 00000000 xpc3250+0x12a31
0012ec98 6104b4e5 01b3b078 0198cd20 00000000 xpc3250+0x15f0f
0012ed48 61050626 00000001 00000000 00000000 js3250!js_Invoke+0x46f
0012eea4 6104b522 01b3b078 0012ef30 00000001 js3250!js_Invoke+0x55b0
0012ef48 6104b77b 00000001 00000001 00000002 js3250!js_Invoke+0x4ac
0012efc8 610349f8 01b3b0a4 01ec8160 01f0fa88 js3250!js_Invoke+0x705
0012eff0 613c4262 01b3b078 01ec8160 01f0fa88 js3250!JS_CallFunctionValue+0x1e
...

ChildEBP RetAddr  Args to Child              
0012e3c4 77f41dc9 00280000 020fe898 0012e464 ntdll!RtlpUnWaitCriticalSection+0x126
0012e498 77bfab2e 00280000 00000000 020fe8a0 ntdll!RtlpUnWaitCriticalSection+0x1cb
0012e4e0 00e02439 020fe8a0 00e2749f 01f52818 msvcrt!free+0xc3
0012e4e8 00e2749f 01f52818 020fe8a0 01f52818 js3250!JS_free+0x11 (FPO: [2,0,0])
0012e51c 00e1a779 01f52818 0214c760 01f52894 js3250!js_FinalizeObject+0x5e
0012e554 00e1a189 01f52818 00000000 0204c148 js3250!js_GC+0x5e8
0012e564 00e02616 01f52818 00000000 0204c148 js3250!js_ForceGC+0x23 (FPO: [2,0,1])
0012e574 010749b6 01f52818 1002955e 0254f5c8 js3250!JS_GC+0x2b (FPO: [1,0,1])
0012e57c 1002955e 0254f5c8 0204c148 00bc1b34 gklayout!nsJSContext__Notify+0xd
(FPO: [2,0,0])
0012e59c 015a9409 00bc1b28 1003d273 00000000 xpcom!nsTimerImpl__Fire+0x77 (FPO:
[0,0,3])
0012e5d4 015841d0 025ba7d8 0012e624 0012e628
gkwidget!nsAppShell__GetNativeEvent+0xa2
0012e63c 01582d60 00000000 00de242b 020f4de4 appshell!nsXULWindow__ShowModal+0x13c
0012e644 00de242b 020f4de4 020c27bc 00000000
appshell!nsContentTreeOwner__ShowAsModal+0x10 (FPO: [1,0,0])
0012e808 00de1aaa 020b5590 020f4de4 01b34f40
embedcomponents!nsWindowWatcher__OpenWindowJS+0x968
0012e83c 00de5e38 00c2e468 01a01e64 00df5a20
embedcomponents!nsWindowWatcher__OpenWindow+0x4f
0012e888 00de549f 00000000 025cc8b0 025cc8b0
embedcomponents!nsPromptService__DoDialog+0xa1
0012e9b8 00de6417 020c27b8 01a01e64 0000000c
embedcomponents!nsPromptService__ConfirmEx+0x32b
0012e9ec 011a5c2e 020c2780 01f255d0 0250eb00
embedcomponents!nsPrompt__ConfirmEx+0x2e
0012ebcc 011a5cbf 01f255d0 0250eb00 020c2748
caps!nsScriptSecurityManager__CheckConfirmDialog+0x30f
0012ebf0 011a5dc1 00bbbe20 00000000 020c2748
caps!nsScriptSecurityManager__RequestCapability+0x3b
0012ed98 011a7fa0 0003be20 020c2748 00000000
caps!nsScriptSecurityManager__EnableCapability+0xc0
0012edc4 00e1b8ce 01fc3010 01a3f8c0 00000000
caps!nsSecurityNameSet__QueryInterface+0x1c3
0012ee78 00e20a2b 00000001 00000001 00000000 js3250!js_Invoke+0x577
...

Comment 15

[Jeff Walden [:Waldo]]

Crashed in 1.7b on Windows.  Talkback ID in status whiteboard.

Comment 16

[Asa Dotzler [:asa]]

Jeff's Talback report says:

ntdll.dll + 0x3336f (0x77f8336f)
ntdll.dll + 0x8b7b (0x77f58b7b)
msvcrt.dll + 0x1ab2e (0x77c2ab2e)
??3@YAXPAX@Z
nsTreeRows::Subtree::Clear [/mozilla/content/xul/templates/src/nsTreeRows.cpp,
line 222]
xptiInterfaceInfo::GetIIDForParamNoAlloc
[/mozilla/xpcom/reflect/xptinfo/src/xptiprivate.h, line 737]
GetInterfaceTypeFromParam [/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 1558]
XPCWrappedNative::CallMethod
[/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2089]
0x0012eb3c

Comment 17

[[e-mail]]

Attachment: Proposed patch

Ah, the problem is that iter is actually invalid when removing the last row :-[

iter.GetRowIndex() >= 0 seems to be the most reliable test.

Comment 18

[[e-mail]]

Comment on attachment 145169 [details] [diff] [review]
Proposed patch

Low-risk intermittent crash regression fix.

Comment 19

[chris hofmann]

Comment on attachment 145169 [details] [diff] [review]
Proposed patch

a=chofmann for 1.7

Comment 20

[[e-mail]]

Fix checked in.