Closed Bug 442227 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: XUL, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: arno)

References

Details

(5 keywords, Whiteboard: [sg:dos] null deref)

Crash Data

Attachments

(3 files)

Attached file 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
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)
As this bug is marked as security, I don't known how to manage it, and who I should ask for a review.
Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

Maybe Smaug can review.
Attachment #327472 - Flags: review?(Olli.Pettay)
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?
Attachment #327472 - Flags: review?(enndeakin)
Attachment #327472 - Flags: review?(Olli.Pettay)
Attachment #327472 - Flags: review+
(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.
Attachment #327472 - Flags: superreview?(neil)
Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

I somehow knew that was coming ;-)
Attachment #327472 - Flags: superreview?(neil) → superreview+
Attached file 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
Patch does not fix this testcase. Therefore, I've opened bug #445177 and will copy CC list.
Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

seems ok
Attachment #327472 - Flags: review?(enndeakin) → review+
Is this patch ready for checkin?
Flags: blocking1.9.1?
Tue Sep 09 13:07:35 2008 +0300 (at Tue Sep 09 13:07:35 2008 +0300)
changeset 19012	af9ae0314398
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: blocking1.9.1?
Assignee: jag → arenevier
1.9.0 !exploitable report
PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!nsFrameManager::GetPrimaryFrameFor
Flags: blocking1.9.0.11?
content/xul/document/crashtests/428951-1.xul was checked in as the testcase for this bug.
Flags: in-testsuite+
This seems to be a consistent null dereference (no prevSibling). Nice to have on 1.9.0.x but not a blocker.
Flags: blocking1.9.0.11? → wanted1.9.0.x+
Whiteboard: [sg:dos] null deref
Flags: blocking1.9.0.12+
Attachment #327472 - Flags: approval1.9.0.12?
Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

This applied fine to 1.9.0, requesting approval.
Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

Approved for 1.9.0.12, a=dveditz for release-drivers
Attachment #327472 - Flags: approval1.9.0.12? → approval1.9.0.12+
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
Group: core-security
Keywords: fixed1.9.0.12
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.
Does not affect 1.8.
Flags: wanted1.8.1.x-
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
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsFrameManager::GetPrimaryFrameFor]
You need to log in before you can comment on or make changes to this bug.