442227 - Crash [@ nsFrameManager::GetPrimaryFrameFor] with mathml, DOMAttrModified doing stuff

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build after 200ms.
It also crashes Firefox 3.

This regressed between 2008-01-29 and 2008-01-30:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-01-29+04&maxdate=2008-01-30+09&cvsroot=%2Fcvsroot
I think a regression from bug 391002, somehow.

Stack trace from debug build:
>	gklayout.dll!nsFrameManager::GetPrimaryFrameFor(nsIContent * aContent=0x066d6cf0, int aIndexHint=2)  Line 377 + 0xb bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ContentRemoved(nsIContent * aContainer=0x012eb140, nsIContent * aChild=0x066d6cf0, int aIndexInContainer=2, int * aDidReconstruct=0x0012e74c)  Line 9485 + 0x1d bytes	C++
 	gklayout.dll!PresShell::ContentRemoved(nsIDocument * aDocument=0x050578e8, nsIContent * aContainer=0x012eb140, nsIContent * aChild=0x066d6cf0, int aIndexInContainer=2)  Line 4766	C++
 	gklayout.dll!nsNodeUtils::ContentRemoved(nsINode * aContainer=0x012eb140, nsIContent * aChild=0x066d6cf0, int aIndexInContainer=2)  Line 167 + 0xe6 bytes	C++
 	gklayout.dll!nsGenericElement::doRemoveChildAt(unsigned int aIndex=2, int aNotify=1, nsIContent * aKid=0x066d6cf0, nsIContent * aParent=0x012eb140, nsIDocument * aDocument=0x050578e8, nsAttrAndChildArray & aChildArray={...})  Line 2846 + 0x11 bytes	C++
 	gklayout.dll!nsGenericElement::RemoveChildAt(unsigned int aIndex=2, int aNotify=1)  Line 2776 + 0x2a bytes	C++
 	gklayout.dll!nsXULElement::RemoveChildAt(unsigned int aIndex=2, int aNotify=1)  Line 1029 + 0x13 bytes	C++
 	gklayout.dll!nsGenericElement::doRemoveChild(nsIDOMNode * aOldChild=0x066d6d0c, nsIContent * aParent=0x012eb140, nsIDocument * aDocument=0x050578e8, nsIDOMNode * * aReturn=0x0012eb28)  Line 3438 + 0x13 bytes	C++
 	gklayout.dll!nsGenericElement::RemoveChild(nsIDOMNode * aOldChild=0x066d6d0c, nsIDOMNode * * aReturn=0x0012eb28)  Line 3004 + 0x1a bytes	C++
 	gklayout.dll!nsXULElement::RemoveChild(nsIDOMNode * oldChild=0x066d6d0c, nsIDOMNode * * _retval=0x0012eb28)  Line 614 + 0x14 bytes	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000011, unsigned int methodIndex=2, unsigned int paramCount=1239832, nsXPTCVariant * params=0x00000000)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=17)  Line 2393 + 0x21 bytes	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2393 + 0x21 bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x04d2a5d8, JSObject * obj=0x023000e0, unsigned int argc=1, long * argv=0x034935d0, long * vp=0x0012ede8)  Line 1473 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x04d2a5d8, unsigned int argc=1, long * vp=0x034935c8, unsigned int flags=2)  Line 1297 + 0x20 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x04d2a5d8)  Line 4852 + 0x16 bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x04d2a5d8, unsigned int argc=1, long * vp=0x034935b8, unsigned int flags=0)  Line 1313 + 0x9 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x04d2a5d8, JSObject * obj=0x02232900, long fval=36766528, unsigned int flags=0, unsigned int argc=1, long * argv=0x04dd0e38, long * rval=0x0012f628)  Line 1369 + 0x15 bytes	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x04d2a5d8, JSObject * obj=0x02232900, long fval=36766528, unsigned int argc=1, long * argv=0x04dd0e38, long * rval=0x0012f628)  Line 5054 + 0x1f bytes	C
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x04a88358, void * aScope=0x02232900, void * aHandler=0x02310340, nsIArray * aargv=0x04f5d474, nsIVariant * * arv=0x0012f6e0)  Line 1962 + 0x24 bytes	C++
 	gklayout.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout=0x04d66228)  Line 7897 + 0xab bytes	C++
 	gklayout.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer=0x04b1a468, void * aClosure=0x04d66228)  Line 8231	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 400 + 0xe bytes	C++
 	xpcom_core.dll!nsTimerEvent::Run()  Line 492	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f848)  Line 511	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x012b4068, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 170 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 181 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x003ff7c0, const nsXREAppData * aAppData=0x003ffe68)  Line 3170 + 0x25 bytes	C++
 	firefox.exe!NS_internal_main(int argc=1, char * * argv=0x003ff7c0)  Line 158 + 0x12 bytes	C++
 	firefox.exe!wmain(int argc=1, unsigned short * * argv=0x003fa080)  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

[arno renevier]

What is the point to call SynchronizeBroadcastListener inside RemoveBroadcastListenerFor ?

That causes the attribute to be set again, and a new notification to be triggered (because style attribute has changed from  "-moz-background-clip: -moz-initial; -moz-background-origin: -moz-initial; -moz-background-inline-policy: -moz-initial;" to "background: -moz-initial -moz-initial -moz-initial -moz-initial -moz-initial; -moz-background-clip: -moz-initial; -moz-background-origin: -moz-initial; -moz-background-inline-policy: -moz-initial;". At some point, that causes the crash.

by removing SynchronizeBroadcastListener call at the end inside RemoveBroadcastListenerFor that fixes the problem, and I could not think of a case where that would cause a problem. (I've run tests, and they're all fine with that modification)

Comment 2

[arno renevier]

Attachment: do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

Comment 3

[arno renevier]

As this bug is marked as security, I don't known how to manage it, and who I should ask for a review.

Comment 4

[Martijn Wargers (dead)]

Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

Maybe Smaug can review.

Comment 5

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

Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

I tried to find some reason for that SynchronizeBroadcastListener, but couldn't.
The listener should be synchronized already before RemoveBroadcastListenerFor call.
Neil, can you think any reason why we'd need to synchronize when removing a broadcast listener?

Comment 6

[[e-mail]]

(In reply to comment #5)
> Neil, can you think any reason why we'd need to synchronize when removing a
> broadcast listener?
No, it seems to have been inadvertently added in bug 26104, and the reviewer didn't spot it either :-( Ironically an attempt was made to improve on it in bug 391002 but that had to be backed out in bug 414907.

Comment 7

[[e-mail]]

Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

I somehow knew that was coming ;-)

Comment 8

[Martijn Wargers (dead)]

Attachment: testcase2

I'm assuming this will be fixed too by the patch, since this testcase has the same regression range.

http://crash-stats.mozilla.com/report/index/aa1d338f-50f9-11dd-8081-001a4bd43ef6
0  	xul.dll  	nsContentUtils::ComparePosition  	 content/base/src/nsContentUtils.cpp:1536
1 	xul.dll 	nsContentUtils::PositionIsBefore 	obj-firefox/dist/include/content/nsContentUtils.h:277
2 	xul.dll 	nsContentList::ContentAppended 	content/base/src/nsContentList.cpp:566

Comment 9

[arno renevier]

Patch does not fix this testcase. Therefore, I've opened bug #445177 and will copy CC list.

Comment 10

[Neil Deakin]

Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

seems ok

Comment 11

[Neil Deakin]

Is this patch ready for checkin?

Comment 12

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

Tue Sep 09 13:07:35 2008 +0300 (at Tue Sep 09 13:07:35 2008 +0300)
changeset 19012	af9ae0314398

Comment 13

[Bob Clary [:bc] (inactive)]

1.9.0 !exploitable report
PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!nsFrameManager::GetPrimaryFrameFor

Comment 14

[Bob Clary [:bc] (inactive)]

content/xul/document/crashtests/428951-1.xul was checked in as the testcase for this bug.

Comment 15

[Daniel Veditz [:dveditz]]

This seems to be a consistent null dereference (no prevSibling). Nice to have on 1.9.0.x but not a blocker.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

This applied fine to 1.9.0, requesting approval.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 18

[Daniel Veditz [:dveditz]]

On the 1.9.0 branch

Checking in content/xul/document/crashtests/428951-1.xul;
/cvsroot/mozilla/content/xul/document/crashtests/428951-1.xul,v  <--  428951-1.xul
initial revision: 1.1
done
Checking in content/xul/document/crashtests/crashtests.list;
/cvsroot/mozilla/content/xul/document/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.3; previous revision: 1.2
done
Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.836; previous revision: 1.835
done

Comment 19

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

Verified fixed for 1.9.0.12 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009070105 GranParadiso/3.0.12pre (.NET CLR 3.5.30729)). Neither of the testcases crash there as they do when opened in 1.9.0.11.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Does not affect 1.8.

Comment 21

[Aakash Desai [:aakashd]]

verified FIXED using the testcases provided on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre ID:20090721044139

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre ID:20090721044139