316159 - crash when XMLHttpRequest target uses xlink:href="" without binding namespace [@ JS_GetScriptPrincipals]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Andre Winter]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

FF1.5b2 crashes when XMLHttpRequest gets a document fragment that holds <use xlink:href=""/>s AND when the xlink-NS isn't set in the outermost tag of this fragment. i know that it should be, but on the other hand, FF should not crash based on such an error.

i have set up a sample at <http://svg.carto.net/temp/91462_load.svg> onclick you are able to load twice the same streets layer xml-file (with different strokes and the difference described above. the XML-files are <http://svg.carto.net/temp/99924_xlink_set.xml> and <http://svg.carto.net/temp/99924_xlink_notset.xml>.

Reproducible: Always

Steps to Reproduce:
1. XMLHttpRequest inside loaded SVG doc
2. missing XLINk-declaration on loaded XML-data
3. crash

Actual Results:  
crash

Expected Results:  
error message and/or ignore.

Comment 1

[Jonathan Watt [:jwatt]]

js3250.dll!JS_GetScriptPrincipals(JSContext * cx=0x01f69198, JSScript * script=0xec010751)  Line 651 + 0x4	C
caps.dll!nsScriptSecurityManager::GetScriptPrincipal(JSContext * cx=0x01f69198, JSScript * script=0xec010751, unsigned int * rv=0x0012f440)  Line 1911	C++
caps.dll!nsScriptSecurityManager::GetFramePrincipal(JSContext * cx=0x01f69198, JSStackFrame * fp=0x0012f654, unsigned int * rv=0x0012f440)  Line 1992 + 0xa	C++
caps.dll!nsScriptSecurityManager::GetPrincipalAndFrame(JSContext * cx=0x0012f654, JSStackFrame * * frameResult=0x0012f41c, unsigned int * rv=0x0012f440)  Line 2026 + 0xa	C++
caps.dll!nsScriptSecurityManager::GetSubjectPrincipal(JSContext * cx=0x01f69198, unsigned int * rv=0x0012f440)  Line 2068 + 0xf	C++
caps.dll!nsScriptSecurityManager::doGetSubjectPrincipal(unsigned int * rv=0x0012f440)  Line 1666 + 0xa	C++
caps.dll!nsScriptSecurityManager::GetSubjectPrincipal(nsIPrincipal * * aSubjectPrincipal=0x0012f460)  Line 1651	C++
caps.dll!nsScriptSecurityManager::SubjectPrincipalIsSystem(int * aIsSystem=0x00000000)  Line 1701	C++
gklayout.dll!nsGlobalWindow::IsCallerChrome()  Line 3144	C++
gklayout.dll!nsGlobalWindow::CanSetProperty(const char * aPrefName=0x0197fc1c)  Line 4053 + 0x5	C++
gklayout.dll!nsGlobalWindow::Focus()  Line 3435 + 0xa	C++
appshell.dll!nsWebShellWindow::HandleEvent(nsGUIEvent * aEvent=0x012ef270)  Line 501	C++
gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f5b0, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1252 + 0x3	C++
gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x00000000)  Line 1273	C++
gkwidget.dll!nsWindow::DispatchFocus(unsigned int aEventType=105, int isMozWindowTakingFocus=1)  Line 6168 + 0xe	C++
gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=7, unsigned int wParam=0, long lParam=0, long * aRetValue=0x0012f898)  Line 4742	C++
gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x00480330, unsigned int msg=7, unsigned int wParam=0, long lParam=19840540)  Line 1434 + 0x10	C++

Comment 2

[Jonathan Watt [:jwatt]]

After clicking on the text to crash I had to switch away from firefox and then back to get it to crash (force it to rerender I guess).

Comment 3

[Johnny Stenback (:jst)]

The real problem here is in frame construction code it seems. I've seem two different ways this crashes, one is a null pointer dereference in nsCSSFrameConstructor.cpp's WipeContainingBlock -> IsFrameSpecial... code, the other one is the crash in caps/js code where we apparently have a bad context, but that's a side effect of the frame construction problem. In the cases where the WipeContainingBlock() code doesn't end up dereferencing a null pointer here it ends up pushing us into lala land and stomps over the stack etc, and we later on crash in caps and/or JS code. The stacks from the caps/js crashes are useless, but they always go through the same codepath that sometimes leads to a straight forward null pointer dereference, and the stack for that is here:

	nsIFrame::GetStateBits() Line 817 + 0xa
	IsFrameSpecial(aFrame=0x00000000) Line 451 + 0x8
	nsCSSFrameConstructor::WipeContainingBlock(aState={...}, aContainingBlock=0x00000000, aFrame=0x035b0834, aFrameList=0x035bc548) Line 13394 + 0x9
	nsCSSFrameConstructor::ContentAppended(aContainer=0x03589c50, aNewIndexInContainer=0) Line 8755 + 0x1e
	PresShell::ContentAppended(aDocument=0x03486648, aContainer=0x03589c50, aNewIndexInContainer=0) Line 5456
	nsDocument::ContentAppended(aContainer=0x03589c50, aNewIndexInContainer=0) Line 2357
	doInsertChildAt(aKid=0x0348b850, aIndex=0, aNotify=1, aParent=0x03589c50, aDocument=0x03486648, aChildArray={...}) Line 2771
	nsGenericElement::InsertChildAt(aKid=0x0348b850, aIndex=0, aNotify=1) Line 2719 + 0x25
	nsContentOrDocument::InsertChildAt(aKid=0x0348b850, aIndex=0, aNotify=1, aChildArray={...}) Line 2884 + 0x23
	nsGenericElement::doInsertBefore(aNewChild=0x0348b86c, aRefChild=0x00000000, aParent=0x03589c50, aDocument=0x03486648, aChildArray={...}, aReturn=0x0012e2f8) Line 3500 + 0x1b
	nsGenericElement::InsertBefore(aNewChild=0x0348b86c, aRefChild=0x00000000, aReturn=0x0012e2f8) Line 3019 + 0x25
	nsSVGGElement::InsertBefore(aNewChild=0x0348b86c, aRefChild=0x00000000, aReturn=0x0012e2f8) Line 61 + 0x18
	nsGenericElement::AppendChild(aNewChild=0x0348b86c, aReturn=0x0012e2f8) Line 515
	nsSVGGElement::AppendChild(aNewChild=0x0348b86c, aReturn=0x0012e2f8) Line 61 + 0x14
	XPTC_InvokeByIndex(that=0x03589c94, methodIndex=18, paramCount=2, params=0x0012e2e8) Line 102
	XPCWrappedNative::CallMethod(ccx={...}, mode=CALL_METHOD) Line 2139 + 0x1e
	XPC_WN_CallMethod(cx=0x03215770, obj=0x029ea508, argc=1, argv=0x034d403c, vp=0x0012e5c4) Line 1444 + 0xe
	js_Invoke(cx=0x03215770, argc=1, flags=0) Line 1177 + 0x20
	js_Interpret(cx=0x03215770, pc=0x034b897f, result=0x0012f110) Line 3522 + 0xf
	js_Invoke(cx=0x03215770, argc=0, flags=2) Line 1197 + 0x13
	nsXPCWrappedJSClass::CallMethod(wrapper=0x02a5e9c8, methodIndex=3, info=0x018fd128, nativeParams=0x0012f42c) Line 1369 + 0x14
	nsXPCWrappedJS::CallMethod(methodIndex=3, info=0x018fd128, params=0x0012f42c) Line 462
	PrepareAndDispatch(self=0x02a5e9c8, methodIndex=3, args=0x0012f4f4, stackBytesToPop=0x0012f4e4) Line 117 + 0x1c
	SharedStub() Line 147	C++
	nsXMLHttpRequest::ChangeState(aState=16, aBroadcast=1, aClearEventListeners=1) Line 1855 + 0x17

Over to SVG for now, seems like the problem is simply that nsCSSFrameConstructor::WipeContainingBlock() is always passed a null containing block when we see this crash (caps/js or direct null dereference crash)...

Comment 4

[Johnny Stenback (:jst)]

Attachment: Possible fix.

This simply adds a null pointer check and that appears to fix this crash. I have no idea if this is the correct fix tho...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> one is a null pointer dereference in nsCSSFrameConstructor.cpp's
> WipeContainingBlock

That should be fixed as of bug 291902 (on trunk only, not 1.8 branch).  The code this patch is touching is gone on trunk as of bug 314878.

If we feel that we need to do something about this on branch, I'd say the null-check should be in WipeContainingBlock itself, since this is not the only caller that can (and does) pass in a null containing block.

> it ends up pushing us into lala land

This worries me more... is that a result of the null dereference, or something else?  Or do we not know?

Comment 6

[Johnny Stenback (:jst)]

(In reply to comment #5)
> > it ends up pushing us into lala land
> 
> This worries me more... is that a result of the null dereference, or something
> else?  Or do we not know?
> 

Don't know for sure, didn't make complete sense to me as I was looking at it and I didn't get around to figuring out exactly why...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 203880 [details] [diff] [review]
Possible fix.

sr- per comment 5...  If we need to do a null-check on branch here, we should do it in WipeContainingBlock.

Comment 8

[Johnny Stenback (:jst)]

Attachment: Null-check in WipeContainingBlock().

Per bz's comments. I'll request reviews etc if we decide to take this for a 1.5x release...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 204381 [details] [diff] [review]
Null-check in WipeContainingBlock().

For what it's worth, r+sr=bzbarsky for branch if we need to take this.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I think we do want this on branch....

Comment 11

[Daniel Veditz [:dveditz]]

Please land on 1.8 branch for regression testing before asking for 1.8.0.5 approval

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 204381 [details] [diff] [review]
Null-check in WipeContainingBlock().

approved for 1.8.0 branch, a=dveditz for drivers

Comment 13

[Johnny Stenback (:jst)]

Fixed on trunk and the 1.8 and 1.8.0 branches.

Comment 14

[Adam Guthrie]

Andre, could you possibly revive the original testcase you posted so that we can verify this? Also, if anyone else could come up with a testcase that would be great.[

Comment 15

[Andre Winter]

the text case files are up on the server again (rebuild).

i didn't try the patch, but my actual mozilla version (Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.1) Gecko/2006120418 Firefox/2.0.0.1) does the job without crashing and without error message.

now i'm just hoping for enhanced rendering of large svg files (eg. maps) in v.3. :-)