355984 - [FIX]Crash @ nsCSSStyleSheet::DeleteRule: "Who owns the rule?"

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P1)

Description

[Alex Vincent [:WeirdAl]]

Steps to reproduce:
(1) Delete a CSS rule from a stylesheet.

Original testcase:  Attempting to reproduce bug 355931, crashed after accepting terms & conditions.

Testcase suggested by bz coming up.

Comments from bz:
3.349 <[e-mail]> 2005-10-15 13:21
Convert CSSGroupRule and CSSStyleSheet to use nsCOMArray. Bug 312491, r+sr=bzbarsky.
Is what it's a regression from
Here's the problem
1685 nsICSSRule* rule = mInner->mOrderedRules.ObjectAt(aIndex);
1686 if (rule) {
1687 mInner->mOrderedRules.RemoveObjectAt(aIndex);
1688 rule->SetStyleSheet(nsnull);
Who owns the rule?
document.styleSheets[0].deleteRule(0)
In any document with a stylesheet that has some rules
that nsICSSRule* needs to be an nsCOMPtr<nsICSSRule>
Otherwise the last ref to the rule might be held by mInner->mOrderedRules
And the rule might die when we RemoveObjectAt

Stack:

00 gklayout!nsCSSStyleSheet::DeleteRule(unsigned int aIndex = 0x33)+0x10a [m:\trunk\mozilla\layout\style\nscssstylesheet.cpp @ 1688]
01 xpcom_core!XPTC_InvokeByIndex(class nsISupports * that = 0x054b666c, unsigned int methodIndex = 0xe, unsigned int paramCount = 1, struct nsXPTCVariant * params = 0x0012dde8)+0x27 [m:\trunk\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp @ 102]
02 xpc3250!XPCWrappedNative::CallMethod(class XPCCallContext * ccx = 0x054b666c, XPCWrappedNative::CallMode mode = 14 (No matching enumerant))+0xe41 [m:\trunk\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp @ 2162]
03 xpc3250!XPCWrappedNative::CallMethod(class XPCCallContext * ccx = 0x0012df9c, XPCWrappedNative::CallMode mode = CALL_METHOD (0))+0xe41 [m:\trunk\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp @ 2162]
04 xpc3250!XPC_WN_CallMethod(struct JSContext * cx = 0x03aff4f8, struct JSObject * obj = 0x0580b1c8, unsigned int argc = 1, long * argv = 0x0585a55c, long * vp = 0x0012e0b8)+0x177 [m:\trunk\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp @ 1455]
05 js3250!js_Invoke(struct JSContext * cx = 0x03aff4f8, unsigned int argc = 1, unsigned int flags = 0)+0xe47 [m:\trunk\mozilla\js\src\jsinterp.c @ 1377]
06 js3250!js_Interpret(struct JSContext * cx = 0x03aff4f8, unsigned char * pc = 0x0678d86c ":", long * result = 0x0012ebfc)+0xda34 [m:\trunk\mozilla\js\src\jsinterp.c @ 3927]
07 js3250!js_Invoke(struct JSContext * cx = 0x03aff4f8, unsigned int argc = 6, unsigned int flags = 6)+0xeba [m:\trunk\mozilla\js\src\jsinterp.c @ 1396]
08 js3250!fun_apply(struct JSContext * cx = 0x03aff4f8, struct JSObject * obj = 0x057a11e0, unsigned int argc = 6, long * argv = 0x0585a3ac, long * rval = 0x0012ecf0)+0x391 [m:\trunk\mozilla\js\src\jsfun.c @ 1687]
09 js3250!js_Invoke(struct JSContext * cx = 0x03aff4f8, unsigned int argc = 2, unsigned int flags = 0)+0xe47 [m:\trunk\mozilla\js\src\jsinterp.c @ 1377]
0a js3250!js_Interpret(struct JSContext * cx = 0x03aff4f8, unsigned char * pc = 0x06398107 ":", long * result = 0x0012f834)+0xda34 [m:\trunk\mozilla\js\src\jsinterp.c @ 3927]
0b js3250!js_Invoke(struct JSContext * cx = 0x03aff4f8, unsigned int argc = 1, unsigned int flags = 2)+0xeba [m:\trunk\mozilla\js\src\jsinterp.c @ 1396]
0c js3250!js_InternalInvoke(struct JSContext * cx = 0x03aff4f8, struct JSObject * obj = 0x0564c290, long fval = 91889112, unsigned int flags = 0, unsigned int argc = 1, long * argv = 0x06052850, long * rval = 0x0012f9b0)+0x118 [m:\trunk\mozilla\js\src\jsinterp.c @ 1471]
0d js3250!JS_CallFunctionValue(struct JSContext * cx = 0x03aff4f8, struct JSObject * obj = 0x0564c290, long fval = 91889112, unsigned int argc = 1, long * argv = 0x06052850, long * rval = 0x0012f9b0)+0x23 [m:\trunk\mozilla\js\src\jsapi.c @ 4419]
0e gklayout!nsJSContext::CallEventHandler(class nsISupports * aTarget = 0x06781378, void * aScope = 0x0564c290, void * aHandler = 0x057a1dd8, class nsIArray * aargv = 0x0564dc5c, class nsIVariant ** arv = 0x0012fa6c)+0x409 [m:\trunk\mozilla\dom\src\base\nsjsenvironment.cpp @ 1745]
0f gklayout!nsGlobalWindow::RunTimeout(struct nsTimeout * aTimeout = 0x06096118)+0x611 [m:\trunk\mozilla\dom\src\base\nsglobalwindow.cpp @ 6649]
10 gklayout!nsGlobalWindow::TimerCallback(class nsITimer * aTimer = 0x06f46138, void * aClosure = 0x06096118)+0x28 [m:\trunk\mozilla\dom\src\base\nsglobalwindow.cpp @ 6980]
11 xpcom_core!nsTimerImpl::Fire(void)+0x233 [m:\trunk\mozilla\xpcom\threads\nstimerimpl.cpp @ 383]
12 xpcom_core!nsTimerEvent::Run(void)+0xa1 [m:\trunk\mozilla\xpcom\threads\nstimerimpl.cpp @ 458]
13 xpcom_core!nsThread::ProcessNextEvent(int mayWait = 1, int * result = 0x0012fbc4)+0x1a0 [m:\trunk\mozilla\xpcom\threads\nsthread.cpp @ 483]
14 xpcom_core!NS_ProcessNextEvent_P(class nsIThread * thread = 0x00b3b428, int mayWait = 1)+0x56 [m:\trunk\xpg-fx-debug\xpcom\build\nsthreadutils.cpp @ 225]
15 gkwidget!nsBaseAppShell::Run(void)+0x5d [m:\trunk\mozilla\widget\src\xpwidgets\nsbaseappshell.cpp @ 153]
16 tkitcmps!nsAppStartup::Run(void)+0x6b [m:\trunk\mozilla\toolkit\components\startup\src\nsappstartup.cpp @ 171]
17 xul!XRE_main(int argc = 1, char ** argv = 0x00b37d38, struct nsXREAppData * aAppData = 0x004036b0)+0x1d8c [m:\trunk\mozilla\toolkit\xre\nsapprunner.cpp @ 2465]
18 firefox!main(int argc = 1, char ** argv = 0x00b37d38)+0x16 [m:\trunk\mozilla\browser\app\nsbrowserapp.cpp @ 61]
19 firefox!__tmainCRTStartup(void)+0x1a6 [f:\rtm\vctools\crt_bld\self_x86\crt\src\crtexe.c @ 586]
1a firefox!mainCRTStartup(void)+0xd [f:\rtm\vctools\crt_bld\self_x86\crt\src\crtexe.c @ 403]
1b kernel32!BaseProcessStart+0x23

Comment 1

[Alex Vincent [:WeirdAl]]

Hmm, unable to reproduce with a super-minimal testcase on local file system.  :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This might not crash, but just try running under valgrind or stepping through deleteRule and seeing what things look like!

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is a must-fix for 1.9 -- deleted object access and all.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Security-sensitive, per reporter's request.  And he's right -- if |rule| dies before we call SetStyleSheet, we're making a virtual call on a deleted object, which is generally Bad (TM).  Not to mention that we're passing this object around to other code that can call virtual methods on it.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 241668 [details] [diff] [review]
Fix

r+sr=dbaron, although I'm not sure you need the comment -- it ought to be pretty obvious

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed.  Opening up the bug, since this is trunk-only and now fixed.

Comment 8

[Daniel Veditz [:dveditz]]

Thanks for finding and reporting this