205735 - Browser freeze/crash on this page after few clicks, xbl implementation issue - M17x FF10 [@ nsXBLPrototypeHandler::GetEventName] [@ nsXBLInsertionPoint::GetDefaultContent]

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030513 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030513 Mozilla Firebird/0.6

http://home.hccnet.nl/m.wargers/Fotos/thumbs/fotoalbum.htm
Directory listing:
http://home.hccnet.nl/m.wargers/Fotos/thumbs/

This is a photo-album script which I made a time ago. Works for older builds
(Moz 1.3)
It uses a hidden frame to get the directory listing of the directory of photos
you've chosen.
Also, I use a lot of timers and quite some innerHTML.
The border is generated with an xbl binding.

When you browse the photo-album, after a few clicks (two or three at most) the
browser (Mozilla 1.4b) freezes and will eventually crash.

I can't seem to make a simplified test case.
I've tried it in this place:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/foto/fotoalbum.htm
http://home.hccnet.nl/m.wargers/test/mozilla/crash/foto/

This crashes still, but by removing the binding it won't crash anymore.
Removing any more tags, and it won't crash anymore (or a lot less, I'm not sure). 

I have posted it before in the Mozilla bugs forum:
http://www.mozillazine.org/forums/viewtopic.php?t=10455&sid=2a97fb83653b397e858cb4090beb366f

I've managed to squeeze the moments between build (moz 1.4a) times.
The build at date 150403 doesn't crash:
(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/2003041508)
The build at date 160403 does crash:
(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/2003041609)

Reproducible: Always

Steps to Reproduce:
1.Visit page
2.Click on some links 
3.Freeze and eventually crash

Actual Results:  
Freeze and crash

Expected Results:  
It should show the pictures, with the name of the picture (like in Mozilla1.3)

Comment 1

[Adam Hauner]

Reporter: Could you provide TalkBack incident ID for crash?

Comment 2

[Martijn Wargers (dead)]

TB20111456Y at 15-05-2003 10:03
(Took me a while how to get a talkback ID)

Also Windows is bugging me with an error message with this:
ModName: gklayout.dll Offset: 001434ab
Here is that report:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/foto/appcompat.txt

Comment 3

[Martijn Wargers (dead)]

I think I've got the cause of the problem:

In my xbl-file rondehoek2.xml, I have this in my implementation tag:
document.getAnonymousNodes(this)[1].innerHTML='<tr><td><div class="lb"><div 
class="rb"><div class="boven"></div></div></div></td></tr>';

Removing this and no crashing occurs.
Only builds at/after build-date 160403 will crash.

I've removed that line in the original example, that won't crash anymore 
(http://home.hccnet.nl/m.wargers/Fotos/thumbs/fotoalbum.htm)

I've made a new example, one with that line of script in the implementation:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/foto/competitie.htm
(rondehoek2.xml is part of that)
One without that line of script in the implementation:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/foto/competitie2.htm
(rondehoek3.xml is part of that)

The first one is crashing, the second one not. (press refresh once or multiple 
times for a crash)
Directory listing:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/foto/

What I find strange is that overriding the -moz-binding doesn't work nicely at 
the website. You get to see distorted borders (and not fully loaded). When you 
copy the file competitie.htm and rondehoek2.xml to your computer, and you view 
them there, it looks alright. 
A different bug?
Still crashing though in both cases after pressing refresh.

Don't know what I'm doing wrong, but I can't seem to get any talkback incident 
ID in this case.

Comment 4

[Rainer Bielefeld]

WFM with Mozilla/5.0 (Windows; U; Win98; de-AT; rv:1.4b) Gecko/20030507

Comment 5

[Martijn Wargers (dead)]

It is still crashing for me.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030526

I'm changing the title to something including the word 'xbl' 
and 'implementation' in it (which I forgot the last time).

Comment 6

[Martijn Wargers (dead)]

Got a talkback ID: TB21966890H
Used the nightly Mozilla on a Win2000 system.

Comment 7

[Scott Baker]

Bug Day: 1-13-2004
Tested With: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031210

I can confirm the mozilla crash at:
http://home.hccnet.nl/m.wargers/test/mozilla/crash/foto/fotoalbum.htm

if you click on the Club eten 2000 link.

Comment 8

[Asa Dotzler [:asa]]

->XBL

Comment 9

[Andreas Kunz]

Strange crasher... does not really crash my Win2k debug build, but brings it in
a state where loading of this page stalls and some other things do not work
anymore. Does not hang, though, not eat CPU and not change RAM usage.

I only crashed when I closed the tab with the - still loading - page.

Don't know if the stack is helpful, but maybe...:

ns_if_addref(nsIContent * 0xdddddddd) line 114 + 9 bytes
nsXBLInsertionPoint::GetDefaultContent() line 66 + 9 bytes
ChangeDocumentForDefaultContent(nsHashKey * 0x25e27ec0, void * 0x25e27e88, void
* 0x00000000) line 488 + 12 bytes
hashEnumerate(PLDHashTable * 0x25c62fe8, PLDHashEntryHdr * 0x25e27d04, unsigned
int 1, void * 0x0012cd1c) line 115 + 26 bytes
PL_DHashTableEnumerate(PLDHashTable * 0x25c62fe8, int (PLDHashTable *,
PLDHashEntryHdr *, unsigned int, void *)* 0x009eba40 hashEnumerate(PLDHashTable
*, PLDHashEntryHdr *, unsigned int, void *), void * 0x0012cd1c) line 619 + 34 bytes
nsHashtable::Enumerate(int (nsHashKey *, void *, void *)* 0x021c05e0
ChangeDocumentForDefaultContent(nsHashKey *, void *, void *), void * 0x00000000)
line 303 + 21 bytes
nsXBLBinding::ChangeDocument(nsXBLBinding * const 0x25f15fa0, nsIDocument *
0x25dfc118, nsIDocument * 0x00000000) line 1014
nsBindingManager::ChangeDocumentFor(nsBindingManager * const 0x25d9db80,
nsIContent * 0x042491c0, nsIDocument * 0x25dfc118, nsIDocument * 0x00000000)
line 554
nsGenericElement::SetDocument(nsIDocument * 0x00000000, int 1, int 1) line 1661
nsGenericHTMLElement::SetDocument(nsIDocument * 0x00000000, int 1, int 1) line 1282
nsGenericElement::SetDocumentInChildrenOf(nsIContent * 0x06375b40, nsIDocument *
0x00000000, int 1) line 1640
nsGenericElement::SetDocument(nsIDocument * 0x00000000, int 1, int 1) line 1693
+ 17 bytes
nsGenericHTMLElement::SetDocument(nsIDocument * 0x00000000, int 1, int 1) line 1282
nsHTMLBodyElement::SetDocument(nsIDocument * 0x00000000, int 1, int 1) line 511
nsGenericElement::SetDocumentInChildrenOf(nsIContent * 0x25cb0e88, nsIDocument *
0x00000000, int 1) line 1640
nsGenericElement::SetDocument(nsIDocument * 0x00000000, int 1, int 1) line 1693
+ 17 bytes
nsGenericHTMLElement::SetDocument(nsIDocument * 0x00000000, int 1, int 1) line 1282
nsDocument::SetScriptGlobalObject(nsIScriptGlobalObject * 0x00000000) line 1575
DocumentViewerImpl::Close(DocumentViewerImpl * const 0x25ee2e80) line 1039
nsDocShell::Destroy(nsDocShell * const 0x25cbcc8c) line 3043
nsWebShell::Destroy(nsWebShell * const 0x25cbcc8c) line 1263
nsFrameLoader::Destroy(nsFrameLoader * const 0x25cbcb88) line 373
nsSubDocumentFrame::Destroy(nsSubDocumentFrame * const 0x06a50cbc,
nsIPresContext * 0x037e2c10) line 582
nsFrameList::DestroyFrames(nsIPresContext * 0x037e2c10) line 130
nsContainerFrame::Destroy(nsContainerFrame * const 0x06a50b98, nsIPresContext *
0x037e2c10) line 137
nsLineBox::DeleteLineList(nsIPresContext * 0x037e2c10, nsLineList & {...}) line 303
nsBlockFrame::Destroy(nsBlockFrame * const 0x06a508f0, nsIPresContext *
0x037e2c10) line 298 + 16 bytes
nsAreaFrame::Destroy(nsAreaFrame * const 0x06a508f0, nsIPresContext *
0x037e2c10) line 160
nsFrameList::DestroyFrames(nsIPresContext * 0x037e2c10) line 130
nsContainerFrame::Destroy(nsContainerFrame * const 0x06a5050c, nsIPresContext *
0x037e2c10) line 137
CanvasFrame::Destroy(CanvasFrame * const 0x06a5050c, nsIPresContext *
0x037e2c10) line 244
nsFrameList::DestroyFrames(nsIPresContext * 0x037e2c10) line 130
nsContainerFrame::Destroy(nsContainerFrame * const 0x06a50608, nsIPresContext *
0x037e2c10) line 137
nsBoxFrame::Destroy(nsBoxFrame * const 0x06a50608, nsIPresContext * 0x037e2c10)
line 1065 + 13 bytes
nsFrameList::DestroyFrames(nsIPresContext * 0x037e2c10) line 130
nsContainerFrame::Destroy(nsContainerFrame * const 0x06a50410, nsIPresContext *
0x037e2c10) line 137
ViewportFrame::Destroy(ViewportFrame * const 0x06a50410, nsIPresContext *
0x037e2c10) line 68
FrameManager::Destroy(FrameManager * const 0x06844738) line 485
PresShell::Destroy(PresShell * const 0x0673f1c0) line 1831
DocumentViewerImpl::Hide(DocumentViewerImpl * const 0x037e05d0) line 1519
nsSubDocumentFrame::Destroy(nsSubDocumentFrame * const 0x067dedd0,
nsIPresContext * 0x03664340) line 572
nsFrameList::DestroyFrame(nsIPresContext * 0x03664340, nsIFrame * 0x067dedd0)
line 214
nsBoxFrame::RemoveFrame(nsBoxFrame * const 0x03d1d1d4, nsIPresContext *
0x03664340, nsIPresShell & {...}, nsIAtom * 0x00000000, nsIFrame * 0x067dedd0)
line 1118
FrameManager::RemoveFrame(FrameManager * const 0x035d4570, nsIFrame *
0x03d1d1d4, nsIAtom * 0x00000000, nsIFrame * 0x067dedd0) line 977
nsCSSFrameConstructor::ContentRemoved(nsCSSFrameConstructor * const 0x030f2340,
nsIPresContext * 0x03664340, nsIContent * 0x03cd3128, nsIContent * 0x25f84328,
int 3, int 0) line 9519 + 45 bytes
PresShell::ContentRemoved(PresShell * const 0x036876b8, nsIDocument *
0x030744a8, nsIContent * 0x03cd3128, nsIContent * 0x25f84328, int 3) line 5317 +
39 bytes
nsDocument::ContentRemoved(nsIContent * 0x03cd3128, nsIContent * 0x25f84328, int
3) line 1918
nsXULDocument::ContentRemoved(nsIContent * 0x03cd3128, nsIContent * 0x25f84328,
int 3) line 1225
nsXULElement::RemoveChildAt(unsigned int 3, int 1) line 2065
nsXULElement::RemoveChild(nsXULElement * const 0x03cd3134, nsIDOMNode *
0x25f84334, nsIDOMNode * * 0x0012da78) line 990 + 21 bytes
XPTC_InvokeByIndex(nsISupports * 0x03cd3134, unsigned int 17, unsigned int 2,
nsXPTCVariant * 0x0012da68) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 2022 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x03649438, JSObject * 0x03c6ee70, unsigned int 1,
long * 0x25f64f50, long * 0x0012dd38) line 1272 + 14 bytes
js_Invoke(JSContext * 0x03649438, unsigned int 1, unsigned int 0) line 941 + 23
bytes
js_Interpret(JSContext * 0x03649438, long * 0x0012e66c) line 2962 + 15 bytes
js_Invoke(JSContext * 0x03649438, unsigned int 1, unsigned int 2) line 958 + 13
bytes
js_InternalInvoke(JSContext * 0x03649438, JSObject * 0x03c6f318, long 103044712,
unsigned int 0, unsigned int 1, long * 0x0012e8b4, long * 0x0012e790) line 1035
+ 20 bytes
JS_CallFunctionValue(JSContext * 0x03649438, JSObject * 0x03c6f318, long
103044712, unsigned int 1, long * 0x0012e8b4, long * 0x0012e790) line 3579 + 31
bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x03082040, void * 0x03c6f318,
void * 0x06245668, unsigned int 1, void * 0x0012e8b4, int * 0x0012e8b8) line
1259 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x03cbc330, nsIDOMEvent
* 0x25d08230) line 178 + 69 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03cbc3f0,
nsIDOMEvent * 0x25d08230, nsIDOMEventTarget * 0x25d082c0, unsigned int 8,
unsigned int 7) line 1420 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x03cbc2d8,
nsIPresContext * 0x03664340, nsEvent * 0x0012efac, nsIDOMEvent * * 0x0012ef38,
nsIDOMEventTarget * 0x25d082c0, unsigned int 7, nsEventStatus * 0x0012eff8) line
1513 + 56 bytes
nsXULElement::HandleDOMEvent(nsIPresContext * 0x03664340, nsEvent * 0x0012efac,
nsIDOMEvent * * 0x0012ef38, unsigned int 7, nsEventStatus * 0x0012eff8) line 3024
PresShell::HandleDOMEventWithTarget(PresShell * const 0x03687698, nsIContent *
0x03d07bf8, nsEvent * 0x0012efac, nsEventStatus * 0x0012eff8) line 6189 + 33 bytes
nsButtonBoxFrame::MouseClicked(nsIPresContext * 0x03664340, nsGUIEvent *
0x0012f208) line 179
nsButtonBoxFrame::HandleEvent(nsButtonBoxFrame * const 0x03d1cdbc,
nsIPresContext * 0x03664340, nsGUIEvent * 0x0012f208, nsEventStatus *
0x0012f568) line 151
PresShell::HandleEventInternal(nsEvent * 0x0012f208, nsIView * 0x00000000,
unsigned int 1, nsEventStatus * 0x0012f568) line 6155 + 33 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x03687698, nsEvent *
0x0012f208, nsIFrame * 0x03d1cdbc, nsIContent * 0x03d07bf8, unsigned int 1,
nsEventStatus * 0x0012f568) line 6068 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsIPresContext * 0x03664340,
nsMouseEvent * 0x0012f784, nsEventStatus * 0x0012f568) line 2836 + 66 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x038db990,
nsIPresContext * 0x03664340, nsEvent * 0x0012f784, nsIFrame * 0x03d1cdbc,
nsEventStatus * 0x0012f568, nsIView * 0x02f8e5c8) line 1847 + 23 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f784, nsIView * 0x02f8e5c8,
unsigned int 1, nsEventStatus * 0x0012f568) line 6163 + 49 bytes
PresShell::HandleEvent(PresShell * const 0x036876b4, nsIView * 0x02f8e5c8,
nsGUIEvent * 0x0012f784, nsEventStatus * 0x0012f568, int 1, int & 1) line 6006 +
25 bytes
nsViewManager::HandleEvent(nsView * 0x02f8e5c8, nsGUIEvent * 0x0012f784, int 1)
line 2296
nsView::HandleEvent(nsViewManager * 0x02f8e408, nsGUIEvent * 0x0012f784, int 1)
line 298
nsViewManager::DispatchEvent(nsViewManager * const 0x02f8e408, nsGUIEvent *
0x0012f784, nsEventStatus * 0x0012f678) line 2033 + 23 bytes
HandleEvent(nsGUIEvent * 0x0012f784) line 79
nsWindow::DispatchEvent(nsWindow * const 0x0363fb6c, nsGUIEvent * 0x0012f784,
nsEventStatus & nsEventStatus_eIgnore) line 1048 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f784) line 1069
nsWindow::DispatchMouseEvent(unsigned int 301, unsigned int 0, nsPoint *
0x00000000) line 5191 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 301, unsigned int 0, nsPoint *
0x00000000) line 5446
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 6095649, long *
0x0012fc30) line 3985 + 28 bytes
nsWindow::WindowProc(HWND__ * 0x017e08b2, unsigned int 514, unsigned int 0, long
6095649) line 1330 + 27 bytes
USER32! 77e2a2b8()
USER32! 77e045b1()
USER32! 77e0a752()
nsAppShellService::Run(nsAppShellService * const 0x0195d0b8) line 484
main1(int 1, char * * 0x00262638, nsISupports * 0x01919020) line 1291 + 32 bytes
main(int 1, char * * 0x00262638) line 1678 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e9847c()

In nsXBLInsertionPoint::GetDefaultContent(), defaultContent is 0xdddddddd

Comment 10

[Boris Zbarsky [:bzbarsky]]

So.. the regression range for this bug includes the fix for bug 194834.

As far as I can tell, the insertion point we are accessing is deleted.  In fact,
the whole binding may be deleted (it's hard to tell after-the-crash in gdb).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Out of curiousity, who owns the insertion points?  Is it possible for them to
die before the binding does?

In any case, if someone can create a testcase that doesn't require frames,
clicking on links, etc, that would be great.

Comment 12

[Martijn Wargers (dead)]

I'm sorry. I made quite a mess of this bug. 
Probably, I've narrowed it down to the following cause:
I overwrite a <children/> tag by setting innerHTML on the tag which surrounds
the <children /> tag.
This is bad scripting enough, but it does not crash if the <content> tag
contains this: <div><children/></div>
In this case you get an uncaught exception.
But if the <content> tag contains this:
<div><div><children/></div></div>
it crashes.

Comment 13

[Martijn Wargers (dead)]

Attachment: Crasher by deleting children tag with innerhtml method

If you change: <div><div>binding content<xbl:children/></div></div>
width:<div>binding content<xbl:children/></div>
you won't get a crasher.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, so the problem is indeed that the insertion point data points to DOM nodes
that are quite simply not in the document anymore.  The problem is that when
someone modifies the DOM inside a binding we need to update the insertion point
info or something....

I don't really know how any of this stuff works, yet.  Does anyone other than hyatt?

Comment 15

[Neil Paris]

Crash after refreshing the attachment in Comment #13:Mozilla/5.0 (X11; U; Linux
i686; en-US; rv:1.7b) Gecko/20040316
Slackware-current

Talkback ID is TB11314H
Captured at 04/03/04 at 12:40 PM

Comment 16

[Asa Dotzler [:asa]]

from talkback:

_ZN19nsXBLInsertionPoint17GetDefaultContentEv()
_Z31ChangeDocumentForDefaultContentP9nsHashKeyPvS1_()
_Z13hashEnumerateP12PLDHashTableP15PLDHashEntryHdrjPv()
PL_DHashTableEnumerate()
_ZN11nsHashtable9EnumerateEPFiP9nsHashKeyPvS2_ES2_()
_ZN12nsXBLBinding14ChangeDocumentEP11nsIDocumentS1_()
_ZN16nsBindingManager17ChangeDocumentForEP10nsIContentP11nsIDocumentS3_()
_ZN16nsGenericElement11SetDocumentEP11nsIDocumentii()
_ZN20nsGenericHTMLElement11SetDocumentEP11nsIDocumentii()
_ZN16nsGenericElement23SetDocumentInChildrenOfEP10nsIContentP11nsIDocumenti()
_ZN16nsGenericElement11SetDocumentEP11nsIDocumentii()
_ZN20nsGenericHTMLElement11SetDocumentEP11nsIDocumentii()
_ZN17nsHTMLBodyElement11SetDocumentEP11nsIDocumentii()
_ZN16nsGenericElement23SetDocumentInChildrenOfEP10nsIContentP11nsIDocumenti()
_ZN16nsGenericElement11SetDocumentEP11nsIDocumentii()
_ZN20nsGenericHTMLElement11SetDocumentEP11nsIDocumentii()
_ZN10nsDocument21SetScriptGlobalObjectEP21nsIScriptGlobalObject()
_ZN18DocumentViewerImpl5CloseEv()
_ZN10nsDocShell14SetupNewViewerEP16nsIContentViewer()
_ZN10nsDocShell5EmbedEP16nsIContentViewerPKcP11nsISupports()
_ZN10nsDocShell19CreateContentViewerEPKcP10nsIRequestPP17nsIStreamListener()
_ZN22nsDSURIContentListener9DoContentEPKciP10nsIRequestPP17nsIStreamListenerPi()
_ZN18nsDocumentOpenInfo18TryContentListenerEP21nsIURIContentListenerP10nsIChannel()
_ZN18nsDocumentOpenInfo15DispatchContentEP10nsIRequestP11nsISupports()
_ZN18nsDocumentOpenInfo14OnStartRequestEP10nsIRequestP11nsISupports()
_ZN13nsHttpChannel18CallOnStartRequestEv()
_ZN13nsHttpChannel13ProcessNormalEv()
_ZN13nsHttpChannel15ProcessResponseEv()
_ZN13nsHttpChannel14OnStartRequestEP10nsIRequestP11nsISupports()
_ZN17nsInputStreamPump12OnStateStartEv()
_ZN17nsInputStreamPump18OnInputStreamReadyEP19nsIAsyncInputStream()
_ZN23nsInputStreamReadyEvent12EventHandlerEP7PLEvent()
PL_HandleEvent()
PL_ProcessPendingEvents()
_ZN16nsEventQueueImpl20ProcessPendingEventsEv()
_Z24event_processor_callbackPvi17GdkInputCondition()
_Z17our_gdk_io_invokeP11_GIOChannel12GIOConditionPv()
libglib-1.2.so.0 + 0xe88a (0x402a288a)
libglib-1.2.so.0 + 0xfe75 (0x402a3e75)
libglib-1.2.so.0 + 0x1032c (0x402a432c)
libglib-1.2.so.0 + 0x1055c (0x402a455c)
libgtk-1.2.so.0 + 0x8c083 (0x401ce083)
_ZN10nsAppShell3RunEv()
_ZN17nsAppShellService3RunEv()
_Z5main1iPPcP11nsISupports()
main()
libc.so.6 + 0x15936 (0x403cb936)

Comment 17

[Asa Dotzler [:asa]]

*** This bug has been marked as a duplicate of 205413 ***

Comment 18

[Boris Zbarsky [:bzbarsky]]

Er... Asa, this is a totally different stack from that other bug, crashing in a
totally different function. Even if it were not, that bug has no testcase,
unlike this one.

Comment 19

[Asa Dotzler [:asa]]

sorry, had several bugs open and marked the wrong one.

Comment 20

[Asa Dotzler [:asa]]

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

Comment 21

[Jay Patel [:jay]]

I just crashed with the test url using Mozilla 1.7 rc1 on WinXP:

Incident ID: 33994
Stack Signature	nsXBLPrototypeHandler::GetEventName 55f822d4
Email Address	[e-mail]
Product ID	Mozilla17
Build ID	2004042109
Trigger Time	2004-04-29 12:11:08.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	gklayout.dll + (00120007)
URL visited	http://home.hccnet.nl/m.wargers/test/mozilla/crash/foto/fotoalbum.htm
User Comments	Clicked on 2000 link
Since Last Crash	sec
Total Uptime	sec
Trigger Reason	Access violation
Source File Name
d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp
Trigger Line No.	469
Stack Trace 	
nsXBLPrototypeHandler::GetEventName
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp,
line 469]
ChangeDocumentForDefaultContent
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/xbl/src/nsXBLBinding.cpp,
line 493]
hashEnumerate
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/ds/nsHashtable.cpp,
line 121]
PL_DHashTableEnumerate
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/ds/pldhash.c,
line 620]
nsHashtable::Enumerate
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/ds/nsHashtable.cpp,
line 304]
nsXBLBinding::ChangeDocument
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/xbl/src/nsXBLBinding.cpp,
line 1011]
nsBindingManager::ChangeDocumentFor
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/xbl/src/nsBindingManager.cpp,
line 570]
nsGenericElement::SetDocument
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 1727]
nsGenericHTMLElement::SetDocument
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,
line 1279]
nsGenericElement::SetDocumentInChildrenOf
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 1707]
nsGenericElement::SetDocument
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 1759]
nsGenericHTMLElement::SetDocument
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,
line 1279]
nsHTMLBodyElement::SetDocument
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLBodyElement.cpp,
line 501]
nsGenericElement::SetDocumentInChildrenOf
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 1707]
nsGenericElement::SetDocument
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 1759]
nsGenericHTMLElement::SetDocument
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,
line 1279]
nsDocument::SetScriptGlobalObject
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/base/src/nsDocument.cpp,
line 1646]
DocumentViewerImpl::Close
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/content/base/src/nsDocumentViewer.cpp,
line 1143]
nsDocShell::SetupNewViewer
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/docshell/base/nsDocShell.cpp,
line 4794]
nsDocShell::Embed
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/docshell/base/nsDocShell.cpp,
line 4144]
nsDocShell::CreateContentViewer
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/docshell/base/nsDocShell.cpp,
line 4549]
nsDSURIContentListener::DoContent
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/docshell/base/nsDSURIContentListener.cpp,
line 110]
nsDocumentOpenInfo::TryContentListener
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/uriloader/base/nsURILoader.cpp,
line 711]
nsDocumentOpenInfo::DispatchContent
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/uriloader/base/nsURILoader.cpp,
line 468]
nsDocumentOpenInfo::OnStartRequest
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/uriloader/base/nsURILoader.cpp,
line 326]
nsHttpChannel::CallOnStartRequest
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,
line 638]
nsHttpChannel::OnStartRequest
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,
line 3351]
nsInputStreamPump::OnStateStart
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
line 381]
nsInputStreamPump::OnInputStreamReady
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
line 343]
nsInputStreamReadyEvent::EventHandler
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/io/nsStreamUtils.cpp,
line 215]
PL_HandleEvent
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 674]
PL_ProcessPendingEvents
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 612]
nsEventQueueImpl::ProcessPendingEvents
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/nsEventQueue.cpp,
line 395]

Comment 22

[chris hofmann]

bryner is going to try and help on this as soon as he gets done with another bug
he is working on...  if anyone can pitch in and get started on it that would be
good...   it would be a good one to try and get for 1.7

Comment 23

[Martijn Wargers (dead)]

Attachment: Valid testcase with removechild

I'm now thinking the last testcase is not valid (it certainly isn't working
because I used .innerHTML in an xhtml environment).

I made a new testcase with removeChild, and this one is crashing.

I think there are two issues here:
- dom methods removing <children/> tags, which results in a crash
- .innerHTML should not be working for elements created in an xbl environment
(because xbl can only produce xml elements)

Comment 24

[Asa Dotzler [:asa]]

not going to block on this but we'd certainly consider a reviewed patch.

Comment 25

[Jay Patel [:jay]]

Updating summary with M17rc2 since the most recent testcase is crashing for me
with Mozilla 1.7 rc2.

Comment 26

[Jay Patel [:jay]]

Updating summary with lastest releases that are crashing.  Marking this
topcrash+ since I as able to crash again using the simplified testcase
http://bugzilla.mozilla.org/attachment.cgi?id=147881&action=view

My incident:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=476855

Nominating for blocking-aviary1.0, although this might be too much of an edge
case to consider a blocker (I don't see a lot of these types of crashes in
recent Talkback data).

Comment 27

[chris hofmann]

we should try and get this for 1.0.  bryner can you help?

Comment 28

[Asa Dotzler [:asa]]

This isn't something we're likely to see in the wild so setting to blocking
minus. If someone comes up with a patch, we'd consider that for approval.

Comment 29

[Egbert Gerber]

Asa, you're probably right stating this is unlikely to be encountered in the wild.
There might be a rather general problem with people not involved in the
development process wondering why bugs of this kind do not get top-priority.
From my (here rather un-technical) point of view it feels bad to see how easily
a simple snippet of client side code can crash FF.
Let's hope no competitor is preparing their website accordingly ... SFX
campaigners wouldn't feel too happy ;-)

Comment 30

[Jay Patel [:jay]]

I'm still crashing with the testcase using Firefox 1.0 RC2.

Comment 31

[timeless]

[e-mail]: there are very few if any people who know this code, expecting
everyone or really anyone to stop working on whatever part of mozilla that they
know, just so that they can pick up a very complicated, fragile and rather
obscure portion of mozilla code to try and fix a crash is unreasonable. if
you're interested in helping, we /might/ be able to help you help us fix this,
otherwise you'll have to be patient while someone grows enough knowledge and
arranges the time to work on this.

Comment 32

[Hermann Schwab]

wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050218

1st (invalid) testcase doesn´t crash on load, reload, shift-reload
2nd ( valid ) testcase doesn´t crash on load, reload, shift-reload
All other URL´s mentioned here give a 404.

Comment 33

[Boris Zbarsky [:bzbarsky]]

The second testcase crashes quite trivially on reload in a clean debug build
from this morning.

In an opt build it may take a lot of work, depending on exact memory layout,
etc, etc (we're accessing deallocated memory, but we may be able to get away
with it for a bit depending on what else writes to that memory and when).

Comment 34

[Boris Zbarsky [:bzbarsky]]

Note that one hackaround, I suspect, would be to hold strong refs to the content
nodes in the insertion points.  That'd at least keep the objects alive, even if
it wouldn't make them do anything useful.

Comment 35

[Jesse Ruderman]

bz, want to try something like in comment 34?  Accessing deallocated memory is bad, and XBL is very under-tested as a result of this bug.

Comment 36

[Boris Zbarsky [:bzbarsky]]

If you want me to drop everything else for a week or so, sure.  Let me know.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Ok, so comment 34 is on crack.  When I crash on this testcase, I have:

#0  0xb70d2b2a in ns_if_addref<nsIContent*> (expr=0x18)
    at ../../../../../../dist/include/xpcom/nsISupportsUtils.h:114
#1  0xb7579fcb in nsXBLInsertionPoint::GetDefaultContent (this=0xb3917678)
    at ../../../../mozilla/content/xbl/src/nsXBLInsertionPoint.cpp:65
(gdb) frame 1
#1  0xb7579fcb in nsXBLInsertionPoint::GetDefaultContent (this=0xb3917678)
    at ../../../../mozilla/content/xbl/src/nsXBLInsertionPoint.cpp:65
65        NS_IF_ADDREF(defaultContent);
(gdb) p *this
$4 = {mParentElement = 0x0, mIndex = 35281732, mElements = {<nsCOMArray_base> = {
      mArray = {mImpl = 0x8696d10}}, <No data fields>}, mDefaultContentTemplate = {
    mRawPtr = 0xb3954a28}, mDefaultContent = {mRawPtr = 0x18}}

Note that mParentElement (which is the weak pointer around here) is 0 and that mDefaultContent is bogus...

At a guess, at this point |this| is a deleted object.  I guess next stop is valgrind, I guess.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Here's what valgrind has to say:

==13486== Invalid read of size 4
==13486==    at 0x4D411A4: nsCOMPtr<nsIContent>::get() const (nsCOMPtr.h:831)
==13486==    by 0x4D40923: nsCOMPtr<nsIContent>::operator nsDerivedSafe<nsIContent>*() const (nsCOMPtr.h:843)
==13486==    by 0x5213FB9: nsXBLInsertionPoint::GetDefaultContent() (nsXBLInsertionPoint.cpp:64)
==13486==    by 0x51EEBDC: ChangeDocumentForDefaultContent(nsHashKey*, void*, void*) (nsXBLBinding.cpp:421)
...
==13486==  Address 0x9481580 is 16 bytes inside a block of size 20 free'd
==13486==    at 0x4005162: operator delete(void*) (vg_replace_malloc.c:246)
==13486==    by 0x520EDE3: DeleteInsertionPoint(void*, void*) (nsBindingManager.cpp:119)
==13486==    by 0x40A792C: nsVoidArray::EnumerateForwards(int (*)(void*, void*), void*) (nsVoidArray.cpp:678)
==13486==    by 0x520EF94: nsAnonymousContentList::~nsAnonymousContentList() (nsBindingManager.cpp:126)
==13486==    by 0x5031B8C: nsGenericDOMNodeList::Release() (nsGenericDOMNodeList.cpp:56)
...
==13486==    by 0x5213C26: RemoveObjectEntry(PLDHashTable&, nsISupports*) (nsBindingManager.cpp:264)

Comment 39

[Boris Zbarsky [:bzbarsky]]

So we're back to the issue of insertion point ownership.  If they're owned by the binding manager (as seems to be the case), then why are there random refs to them in the mInsertionPointTable of a binding that are not cleared?

This used to work correctly back when insertion points were refcounted; perhaps we should simply go back to that?  :(

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, lets keep things simple!

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We can always use non-virtual addref and release (and inline at least addref).

Comment 42

[Boris Zbarsky [:bzbarsky]]

https://bugzilla.mozilla.org/attachment.cgi?id=118259 is the checkin that removed the refcounting.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Make nsXBLInsertionPoints refcounted

I haven't yet fully tested this, but seems to fix the crash, since 
insertionpoints are kept alive until refcount drops to 0.
With some testing I couldn't immediately see any memory leaks.

I hope you sicking haven't been writing a patch for this. But if you have
and if it is something simpler than this, please re-assign to yourself.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

This is just cleaning up the previous patch a bit.

Since you bryner changed the code to non-refcounting, perhaps you
could review this patch which makes nsXBLInsertion refcounted again.

Comment 45

[Jesse Ruderman]

This patch works very well for me.  The only additional XBL problem I was able to find with some quick testing was bug 342954.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #45)
> The only additional XBL problem I was able to find with some quick 
> testing was bug 342954.

bug 342954 happens with or without the patch

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 227207 [details] [diff] [review]
proposed patch

bz, perhaps you have time to review?

Comment 48

[Boris Zbarsky [:bzbarsky]]

Not until sometime next week, probably.  :(   And for the branch, we want a patch that doesn't change interfaces, no?

Have you tried mailing bryner and asking him to review?

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

Attachment: For branch

Using nsVoidArray and manually AddRefing/Releasing, so no need to change any
interfaces.
This patch could be used also on trunk.

(Bryner said he may not have time to review this soon.)

Comment 50

[Boris Zbarsky [:bzbarsky]]

My problem is that not only do I not have the time, but I also don't _really_ know the code.  To review this I'll need to spend a fair amount of time reading the XBL insertion point stuff to make sure that there aren't bits left over that are assuming they can delete insertion points or something...

If bryner really can't do it, I guess I can try, but eta is at least two weeks at this point.

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

Not going to block for this at this point in the release cycle, although if we can get someone (shaver? jonas?) to review that patch and get it on trunk for a few days of solid baking, we'd consider taking it post beta2.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Beltzner, you guys do realize that at least some of the stacks in this bug indicate this is exploitable, right?  Renominating in case you didn't.

For the record, this is the mail I sent to smaug, bryner, sicking, and jst last night:

Guys, I just spent the last 3-4 hours looking at the patches on bug 205735, and I still don't understand the ownership model of the various arrays floating about, much less of the insertion points themselves....  There's no way I can review the patch given my current level of understanding.

At this point, I very much doubt that I will be in a position to really try to understand this code until at least mid-September, and that's only if I'm lucky.  The problem is that we need this fix on the 1.8.1 and 1.8.0.7 branches -- as things stand, web pages can trigger deleted object access.  I don't know the schedule for 1.8.0.7, but from what I understand mid-to-late-Sept is kinda late in terms of 1.8.1.

Brian, Johnny, you've looked at this code before.  I realize that it's been a while, so I'm not sure how much it helps, but if it does at all and if you have the time to look at this, that would be wonderful.  Jonas, I'm ccing you because you might be interested in this stuff too...  If any of you would be willing to take over this review, that would rock. 

Comment 53

[Mike Beltzner [:beltzner, not reading bugmail]]

Marking blocker based on assertion of exploitability (is that a word)? Someone needs to review this patch, then, since we are now saying we're holding release for this.

Comment 54

[Daniel Veditz [:dveditz]]

Bryner, Jonas, can you please look at these patches?

Comment 55

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 229970 [details] [diff] [review]
For branch

>Index: content/xbl/src/nsBindingManager.cpp
>@@ -93,41 +93,48 @@ public:
>   virtual ~nsAnonymousContentList();
> 
>   // nsIDOMNodeList interface
>   NS_DECL_NSIDOMNODELIST
> 
>   PRInt32 GetInsertionPointCount() { return mElements->Count(); }
> 
>   nsXBLInsertionPoint* GetInsertionPointAt(PRInt32 i) { return NS_STATIC_CAST(nsXBLInsertionPoint*, mElements->ElementAt(i)); }
>-  void RemoveInsertionPointAt(PRInt32 i) { mElements->RemoveElementAt(i); }
>+  void RemoveInsertionPointAt(PRInt32 i) {
>+    nsXBLInsertionPoint* insertionPoint =
>+      NS_STATIC_CAST(nsXBLInsertionPoint*, mElements->ElementAt(i));

You may want to use SafeElementAt here just in case.

>Index: content/xbl/src/nsXBLInsertionPoint.h
>+  nsrefcnt Release()
>+  {
>+    --mRefCnt;
>+    NS_LOG_RELEASE(this, mRefCnt, "nsXBLInsertionPoint");
>+    if (mRefCnt == 0) {
>+      mRefCnt = 1;
>+      delete this;
>+      return 0;
>+    }
>+    return mRefCnt;
>+  }

I would recommend not inlineing release since it's so big. AddRef is fine though.

sr=me with that.

Comment 56

[David :Bienvenu]

Comment on attachment 229970 [details] [diff] [review]
For branch

switching request to jst

Comment 57

[Johnny Stenback (:jst)]

Comment on attachment 229970 [details] [diff] [review]
For branch

r=jst, with what sicking pointed out dealt with.

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 227207 [details] [diff] [review]
proposed patch

I'll check in the "branch" patch also to trunk.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +comments

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 229970 [details] [diff] [review]
For branch

Checked in to trunk.

Comment 61

[Boris Zbarsky [:bzbarsky]]

It really would be good to follow up on trunk with something that doesn't do manual refcounting...

Comment 62

[Daniel Veditz [:dveditz]]

Comment on attachment 235212 [details] [diff] [review]
+comments

approved for 1.8.0 branch, a=dveditz for drivers

Comment 63

[Mike Schroepfer]

Comment on attachment 235212 [details] [diff] [review]
+comments

a=schrep for drivers

Comment 64

[alice nodelman [:alice] [:anode]]

using reduced testcase http://bugzilla.mozilla.org/attachment.cgi?id=147881&action=view, should not crash

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060828 Firefox/1.5.0.7pre

verified 1.8.0.7

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060828 BonEcho/2.0b2

verified 1.8.1b2

Comment 65

[Jesse Ruderman]

I added "Valid testcase with removechild" as a crashtest:
http://hg.mozilla.org/mozilla-central/rev/ef8d238e4a8a

I guess it could be an "== about:blank" reftest, if we're planning to continue allowing scripts to remove insertion points.