461027 - Crash [@ nsContentUtils::ComparePosition] with binding

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes in current trunk build.

This regressed between 2008-06-22 and 2008-06-23:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-06-22+04%3A00%3A00&enddate=2008-06-23+08%3A00%3A00
I think a regression from bug 344258.

Stacktrace from debug build:
 	gklayout.dll!nsContentUtils::GetContextForEventHandlers(nsINode * aNode=0x09af6478, nsIScriptContext * * aContext=0x00000004)  Line 4354 + 0x3 bytes	C++
>	gklayout.dll!nsGenericElement::GetContextForEventHandlers(nsIScriptContext * * aContext=0x00000004)  Line 346 + 0xd bytes	C++
 	gklayout.dll!nsContentUtils::ComparePosition(nsINode * aNode1=0x0883bac8, nsINode * aNode2=0x09af6478)  Line 1533 + 0xf bytes	C++
 	gklayout.dll!nsContentUtils::PositionIsBefore(nsINode * aNode1=0x0883bac8, nsINode * aNode2=0x09af6478)  Line 280 + 0xd bytes	C++
 	gklayout.dll!nsIdentifierMapEntry::AddIdContent(nsIContent * aContent=0x0883bac8)  Line 459 + 0xd bytes	C++
 	gklayout.dll!nsDocument::UpdateIdTableEntry(nsIContent * aContent=0x0883bac8)  Line 2272	C++
 	gklayout.dll!nsXULDocument::AddElementToDocumentPre(nsIContent * aElement=0x0883bac8)  Line 1625	C++
 	gklayout.dll!nsXULDocument::AddSubtreeToDocument(nsIContent * aElement=0x0883bac8)  Line 1705 + 0x12 bytes	C++
 	gklayout.dll!nsXBLBinding::InstallAnonymousContent(nsIContent * aAnonParent=0x0883ba78, nsIContent * aElement=0x0997dd38)  Line 371	C++
 	gklayout.dll!nsXBLBinding::GenerateAnonymousContent()  Line 663	C++
 	gklayout.dll!nsXBLService::LoadBindings(nsIContent * aContent=0x0997dd38, nsIURI * aURL=0x095f2ee8, nsIPrincipal * aOriginPrincipal=0x087028e8, int aAugmentFlag=0, nsXBLBinding * * aBinding=0x0012eb5c, int * aResolveStyle=0x0012eb30)  Line 631	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFrameInternal(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x0997dd38, nsIFrame * aParentFrame=0x095ff008, nsIAtom * aTag=0x012e38bc, int aNameSpaceID=6, nsStyleContext * aStyleContext=0x095ff0d0, nsFrameItems & aFrameItems={...}, int aXBLBaseTag=0)  Line 7424 + 0x53 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFrame(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x0997dd38, nsIFrame * aParentFrame=0x095ff008, nsFrameItems & aFrameItems={...})  Line 7379 + 0x35 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessInlineChildren(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x09945ef0, nsIFrame * aFrame=0x095ff008, int aCanHaveGeneratedContent=1, nsFrameItems & aFrameItems={...}, int * aKidsAllInline=0x0012ede4)  Line 12618 + 0x3a bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructInline(nsFrameConstructorState & aState={...}, const nsStyleDisplay * aDisplay=0x095fecf0, nsIContent * aContent=0x09945ef0, nsIFrame * aParentFrame=0x095fe714, nsStyleContext * aStyleContext=0x095fefb8, int aIsPositioned=0, nsIFrame * aNewFrame=0x095ff008)  Line 12390 + 0x21 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFrameByDisplayType(nsFrameConstructorState & aState={...}, const nsStyleDisplay * aDisplay=0x095fecf0, nsIContent * aContent=0x09945ef0, int aNameSpaceID=6, nsIAtom * aTag=0x012e38c8, nsIFrame * aParentFrame=0x095fe714, nsStyleContext * aStyleContext=0x095fefb8, nsFrameItems & aFrameItems={...}, int aHasPseudoParent=0)  Line 6591 + 0x22 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFrameInternal(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x09945ef0, nsIFrame * aParentFrame=0x095fe714, nsIAtom * aTag=0x012e38c8, int aNameSpaceID=6, nsStyleContext * aStyleContext=0x095fefb8, nsFrameItems & aFrameItems={...}, int aXBLBaseTag=0)  Line 7558 + 0x34 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFrame(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x09945ef0, nsIFrame * aParentFrame=0x095fe714, nsFrameItems & aFrameItems={...})  Line 7379 + 0x35 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ContentInserted(nsIContent * aContainer=0x05ce6050, nsIContent * aChild=0x09945ef0, int aIndexInContainer=0, nsILayoutHistoryState * aFrameState=0x05d58458)  Line 8966	C++
 	gklayout.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent=0x09945ef0)  Line 11087 + 0x25 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList & aChangeList={...})  Line 9825	C++
 	gklayout.dll!PresShell::RecreateFramesFor(nsIContent * aContent=0x09945ef0)  Line 3374 + 0x12 bytes	C++
 	gklayout.dll!nsXBLBindingRequest::DocumentLoaded(nsIDocument * aBindingDoc=0x09738080)  Line 263	C++
 	gklayout.dll!nsXBLStreamListener::Load(nsIDOMEvent * aEvent=0x0964b250)  Line 478	C++
 	gklayout.dll!DispatchToInterface(nsIDOMEvent * aEvent=0x0964b250, nsIDOMEventListener * aListener=0x097924d4, unsigned int (nsIDOMEvent *)* aMethod=0x045c3de0, const nsID & aIID={...})  Line 184 + 0xb bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x00000000, nsEvent * aEvent=0x0012f624, nsIDOMEvent * * aDOMEvent=0x0012f59c, nsISupports * aCurrentTarget=0x09738080, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012f5a0)  Line 1182 + 0x22 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 212	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 271	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x09738080, nsPresContext * aPresContext=0x00000000, nsEvent * aEvent=0x0012f624, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x00000000, nsDispatchingCallback * aCallback=0x00000000)  Line 479 + 0x12 bytes	C++
 	gklayout.dll!nsXMLDocument::EndLoad()  Line 580 + 0x15 bytes	C++
 	gklayout.dll!nsXMLContentSink::DidBuildModel()  Line 381	C++
 	gkparser.dll!nsExpatDriver::DidBuildModel(unsigned int anErrorCode=0, int aNotifySink=1, nsIParser * aParser=0x05dc1758, nsIContentSink * aSink=0x05cc6520)  Line 1311 + 0xe bytes	C++
 	gkparser.dll!nsParser::DidBuildModel(unsigned int anErrorCode=0)  Line 1527 + 0x35 bytes	C++
 	gkparser.dll!nsParser::ResumeParse(int allowIteration=1, int aIsFinalChunk=1, int aCanInterrupt=1)  Line 2283	C++
 	gkparser.dll!nsParser::OnStopRequest(nsIRequest * request=0x097b3ca8, nsISupports * aContext=0x00000000, unsigned int status=0)  Line 2916 + 0x17 bytes	C++
 	gklayout.dll!nsXBLStreamListener::OnStopRequest(nsIRequest * request=0x097b3ca8, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0)  Line 380 + 0x28 bytes	C++
 	necko.dll!nsBaseChannel::OnStopRequest(nsIRequest * request=0x097ddf80, nsISupports * ctxt=0x00000000, unsigned int status=0)  Line 623	C++
 	necko.dll!nsInputStreamPump::OnStateStop()  Line 577	C++
 	necko.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x0863dbc8)  Line 401 + 0xb bytes	C++
 	xpcom_core.dll!nsInputStreamReadyEvent::Run()  Line 112	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f848)  Line 511	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x012d3fe8, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 170 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 182 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x003fffe0, const nsXREAppData * aAppData=0x012d0880)  Line 3265 + 0x25 bytes	C++
 	firefox.exe!NS_internal_main(int argc=1, char * * argv=0x003fffe0)  Line 156 + 0x12 bytes	C++
 	firefox.exe!wmain(int argc=1, unsigned short * * argv=0x003fa2e8)  Line 87 + 0xd bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
 	firefox.exe!wmainCRTStartup()  Line 403	C
 	kernel32.dll!_BaseProcessStart@4()  + 0x23 bytes

Comment 1

[Johnny Stenback (:jst)]

Not blocking, but this is an exploitable crash. Marking security sensitive.

Comment 2

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

Attachment: proposed patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Looks ok, though it woud be even better to have a helper function that we call both here and in binding teardown (where we have to do the same thing, right?)

Comment 4

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

I'll file a followup to add a helper and to make sure we call
undo install properly.

Comment 5

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081220 Minefield/3.2a1pre

Comment 6

[Daniel Veditz [:dveditz]]

Do we need this fix in 3.0? bug 344258 was not landed on the 1.9.0 branch but the nsXBLBinding::GenerateAnonymousContent code this patch touches is the same.

Comment 7

[Daniel Veditz [:dveditz]]

I'll assume yes for now so we don't lose track.

Comment 8

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

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

a191=beltzner

Comment 9

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 10

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

The problem with this is to verify this on 1.9.0.x because the testcase is
for 1.9.1.

Comment 11

[Samuel Sidler (old account; do not CC)]

Olli: Any way to make a 1.9.0 testcase? Regardless, if this makes us safer, we should take it.

Comment 12

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

Trying to create 1.9.0 testcase...

Martijn, perhaps you have some suggestions?

Comment 13

[Martijn Wargers (dead)]

Sorry, I don't know of a way to make it crash in 1.9.0, the original testcase doesn't crash in 1.9.0 either.

Comment 14

[Martijn Wargers (dead)]

However I did find a new case that has a similar stacktrace as this bug, I filed bug 472212 for it.

Comment 15

[Alexander Sack]

1.9+ regression, not wanted on 1.8.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

So there is no real way to verify this fix for 1.9.0.6? I double-checked and it doesn't crash in Firefox 3.0.5.

Comment 17

[Martijn Wargers (dead)]

Yes, afaik, there is no way to verify this fix for 1.9.0.6.

Comment 18

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090225 Firefox and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090225 Shiretoko/3.1b3pre.