Last Comment Bug 442227 - Crash [@ nsFrameManager::GetPrimaryFrameFor] with mathml, DOMAttrModified doing stuff
: Crash [@ nsFrameManager::GetPrimaryFrameFor] with mathml, DOMAttrModified doi...
Status: VERIFIED FIXED
[sg:dos] null deref
: crash, regression, testcase, verified1.9.0.12, verified1.9.1
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: arno renevier
:
Mentors:
Depends on:
Blocks: 391002
  Show dependency treegraph
 
Reported: 2008-06-27 04:48 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
12 users (show)
roc: wanted1.9.1+
dveditz: blocking1.9.0.12+
dveditz: wanted1.9.0.x+
stransky: wanted1.8.1.x-
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (705 bytes, application/vnd.mozilla.xul+xml)
2008-06-27 04:48 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor (2.43 KB, patch)
2008-06-30 13:57 PDT, arno renevier
bugs: review+
enndeakin: review+
neil: superreview+
dveditz: approval1.9.0.12+
Details | Diff | Review
testcase2 (864 bytes, application/vnd.mozilla.xul+xml)
2008-07-13 10:18 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2008-06-27 04:48:25 PDT
Created attachment 327093 [details]
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 2008-06-30 13:57:02 PDT
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 2008-06-30 13:57:55 PDT
Created attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor
Comment 3 arno renevier 2008-07-08 03:38:52 PDT
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 [:mwargers] (not working for Mozilla) 2008-07-08 04:01:20 PDT
Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

Maybe Smaug can review.
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-07-11 00:40:08 PDT
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 neil@parkwaycc.co.uk 2008-07-11 02:28:34 PDT
(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 neil@parkwaycc.co.uk 2008-07-11 03:46:28 PDT
Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

I somehow knew that was coming ;-)
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-07-13 10:18:30 PDT
Created attachment 329326 [details]
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 2008-07-14 10:19:02 PDT
Patch does not fix this testcase. Therefore, I've opened bug #445177 and will copy CC list.
Comment 10 Neil Deakin 2008-07-16 10:15:12 PDT
Comment on attachment 327472 [details] [diff] [review]
do not call SynchronizeBroadcastListener from RemoveBroadcastListenerFor

seems ok
Comment 11 Neil Deakin 2008-08-12 11:55:13 PDT
Is this patch ready for checkin?
Comment 12 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-09-09 03:09:46 PDT
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:] 2009-04-28 11:02:15 PDT
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:] 2009-04-28 16:45:37 PDT
content/xul/document/crashtests/428951-1.xul was checked in as the testcase for this bug.
Comment 15 Daniel Veditz [:dveditz] 2009-05-04 22:58:05 PDT
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] 2009-06-08 17:19:32 PDT
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] 2009-06-10 15:32:00 PDT
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] 2009-06-24 18:05:01 PDT
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 [On PTO until 6/29] 2009-07-01 13:02:54 PDT
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ý 2009-07-08 03:09:33 PDT
Does not affect 1.8.
Comment 21 Aakash Desai [:aakashd] 2009-07-21 09:30:58 PDT
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

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