Last Comment Bug 316159 - crash when XMLHttpRequest target uses xlink:href="" without binding namespace [@ JS_GetScriptPrincipals]
: crash when XMLHttpRequest target uses xlink:href="" without binding namespace...
Status: RESOLVED FIXED
[needs testcase]
: crash, fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 1.8 Branch
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Hixie (not reading bugmail)
Mentors:
http://svg.carto.net/temp/91462_load.svg
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-12 01:17 PST by Andre Winter
Modified: 2007-02-06 07:37 PST (History)
9 users (show)
bzbarsky: blocking1.8.1+
dveditz: blocking1.8.0.5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Possible fix. (1.33 KB, patch)
2005-11-21 18:17 PST, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: superreview-
Details | Diff | Splinter Review
Null-check in WipeContainingBlock(). (1.25 KB, patch)
2005-11-28 14:08 PST, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
bzbarsky: superreview+
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review

Description Andre Winter 2005-11-12 01:17:42 PST
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] (back in October - email directly if necessary) 2005-11-12 10:53:49 PST
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] (back in October - email directly if necessary) 2005-11-12 10:55:28 PST
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, jst@mozilla.com) 2005-11-21 18:07:12 PST
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, jst@mozilla.com) 2005-11-21 18:17:23 PST
Created attachment 203880 [details] [diff] [review]
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 [:bz] 2005-11-21 21:08:43 PST
> 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, jst@mozilla.com) 2005-11-23 18:21:57 PST
(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 [:bz] 2005-11-25 16:58:00 PST
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, jst@mozilla.com) 2005-11-28 14:08:01 PST
Created attachment 204381 [details] [diff] [review]
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 [:bz] 2005-11-28 14:11:34 PST
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 [:bz] 2006-05-04 12:35:33 PDT
I think we do want this on branch....
Comment 11 Daniel Veditz [:dveditz] 2006-06-07 15:37:43 PDT
Please land on 1.8 branch for regression testing before asking for 1.8.0.5 approval
Comment 12 Daniel Veditz [:dveditz] 2006-06-20 14:51:25 PDT
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, jst@mozilla.com) 2006-06-20 17:15:32 PDT
Fixed on trunk and the 1.8 and 1.8.0 branches.
Comment 14 Adam Guthrie 2006-06-21 11:04:05 PDT
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 2007-02-06 07:37:28 PST
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. :-)

Note You need to log in before you can comment on or make changes to this bug.