Closed Bug 342677 Opened 18 years ago Closed 18 years ago

select.options.add(null) crashes [@ JS_GetClass]

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: ddkilzer, Assigned: WeirdAl)

References

Details

(5 keywords)

Crash Data

Attachments

(3 files)

Calling select.options.add(null) crashes Firefox 1.5.0.4 hard.  I'm not sure if this is actually a security issue or not, but I'll assume it is until proven otherwise.

I found this while testing Firefox's behavior while implementing the options.add() function in Apple's WebKit.

http://bugzilla.opendarwin.org/show_bug.cgi?id=9179

I will post a test case next.
This is a test case that crashes Firefox 1.5.0.4.

See also Talkback Incident TB20250155X.
It also crashes latest trunk build on Windows XP SP2 as well, updating summary
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Summary: select.options.add(null) crashes Firefox 1.5.0.4 → select.options.add(null) crashes Firefox
Version: 1.5.0.x Branch → Trunk
Keywords: testcase
Keywords: crash
Assignee: nobody → general
Component: Security → DOM: Mozilla Extensions
Depends on: 193223
Keywords: testcase
OS: All → Mac OS X 10.4
Product: Firefox → Core
QA Contact: firefox → ian
Hardware: All → Macintosh
Summary: select.options.add(null) crashes Firefox → select.options.add(null) crashes Firefox 1.5.0.4
Keywords: testcase
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Summary: select.options.add(null) crashes Firefox 1.5.0.4 → select.options.add(null) crashes
review blame is jst.
Assignee: general → jst
expected results (from ie6) are of course:
Invalid argument.
The crash happened for the first time somewhere in August 2005.
###!!! ASSERTION: bad param: 'aJSObj', file m:/vc8/mozilla/js/src/xpconnect/src/nsXPConnect.cpp, line 643

 	ntdll.dll!7c901230() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]	
 	xpcom_core.dll!Break(const char * aMsg=0x0012da1c)  Line 471	C++
 	xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=1, const char * aStr=0x00d07864, const char * aExpr=0x00d0785c, const char * aFile=0x00d07828, int aLine=643)  Line 350 + 0xc bytes	C++
 	xpc3250.dll!nsXPConnect::GetWrappedNativeOfJSObject(JSContext * aJSContext=0x033579f8, JSObject * aJSObj=0x00000000, nsIXPConnectWrappedNative * * _retval=0x0012df38)  Line 643 + 0x22 bytes	C++
>	gklayout.dll!nsHTMLOptionsCollectionSH::Add(JSContext * cx=0x033579f8, JSObject * obj=0x033efac8, unsigned int argc=1, long * argv=0x0365b104, long * rval=0x0012dfdc)  Line 9666 + 0x39 bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x033579f8, unsigned int argc=1, unsigned int flags=0)  Line 1328 + 0x20 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x033579f8, unsigned char * pc=0x0361cf1c, long * result=0x0012eb44)  Line 4021 + 0xf bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x033579f8, unsigned int argc=1, unsigned int flags=2)  Line 1347 + 0x13 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x033579f8, JSObject * obj=0x0334ec48, long fval=54463856, unsigned int flags=0, unsigned int argc=1, long * argv=0x0365b060, long * rval=0x0012ec98)  Line 1422 + 0x14 bytes	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x033579f8, JSObject * obj=0x0334ec48, long fval=54463856, unsigned int argc=1, long * argv=0x0365b060, long * rval=0x0012ec98)  Line 4348 + 0x1f bytes	C
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x0350aca8, void * aScope=0x0334ec48, void * aHandler=0x033f0d70, nsIArray * aargv=0x0363a8e8, nsIVariant * * arv=0x0012ee0c)  Line 1655 + 0x21 bytes	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x033e4fb0)  Line 211 + 0x62 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x034bba90, nsIDOMEventListener * aListener=0x034bb9e8, nsIDOMEvent * aDOMEvent=0x033e4fb0, nsISupports * aCurrentTarget=0x033576c8, unsigned int aSubType=1, unsigned int aPhaseFlags=6)  Line 1648 + 0x12 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x035f2698, nsEvent * aEvent=0x0012f5b0, nsIDOMEvent * * aDOMEvent=0x0012f0d8, nsISupports * aCurrentTarget=0x033576c8, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012f0dc)  Line 1752	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 335	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 455	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 401 + 0x12 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x033576c8, nsPresContext * aPresContext=0x035f2698, nsEvent * aEvent=0x0012f5b0, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f5ac, nsDispatchingCallback * aCallback=0x00000000, int aTargetIsChromeHandler=0)  Line 575 + 0x10 bytes	C++
 	gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0)  Line 1068 + 0x23 bytes	C++
 	docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x0336735c, nsIChannel * aChannel=0x03638640, unsigned int aStatus=0)  Line 4859	C++
 	docshell.dll!nsWebShell::EndPageLoad(nsIWebProgress * aProgress=0x0336735c, nsIChannel * channel=0x03638640, unsigned int aStatus=0)  Line 975	C++
 	docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x0336735c, nsIRequest * aRequest=0x03638640, unsigned int aStateFlags=131088, unsigned int aStatus=0)  Line 4774	C++
 	docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x0336735c, nsIRequest * aRequest=0x03638640, int aStateFlags=131088, unsigned int aStatus=0)  Line 1232	C++
 	docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x03638640, unsigned int aStatus=0)  Line 865	C++
 	docshell.dll!nsDocLoader::DocLoaderIsEmpty()  Line 761	C++
 	docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x03536c90, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0)  Line 678	C++
 	necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x03536c90, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 685 + 0x2e bytes	C++
 	gklayout.dll!nsDocument::DoUnblockOnload()  Line 4965	C++
 	gklayout.dll!nsUnblockOnloadEvent::Run()  Line 4924	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fe04)  Line 483	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x003d9118, int mayWait=1)  Line 225 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 153 + 0xc bytes	C++
 	appcomps.dll!nsAppStartup::Run()  Line 219	C++
 	seamonkey.exe!main1(int argc=1, char * * argv=0x003d2fb0, nsISupports * nativeApp=0x009cb3b0)  Line 1238 + 0x22 bytes	C++
 	seamonkey.exe!main(int argc=1, char * * argv=0x003d2fb0)  Line 1740 + 0x25 bytes	C++
 	seamonkey.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	seamonkey.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816d4f() 	
 	kernel32.dll!7c8399f3() 	

This leads directly to:
###!!! ASSERTION: bad param: 'obj', file m:/vc8/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 1157

Then we hit the crash as described in comment 1.

Just from reading the context, I'd guess we probably need at nsDOMClassInfo.cpp#9663:

if (!wrapper) {
  nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_INVALID_POINTER);
  return JS_FALSE;
}
Keywords: assertion
Attached patch patch, v1Splinter Review
The returned error code is wrong, but this patch does fix the crash & assertions.

Opinions?
Attachment #226991 - Flags: superreview?(jst)
Attachment #226991 - Flags: review?(jst)
Comment on attachment 226991 [details] [diff] [review]
patch, v1

>   NS_DEFINE_CLASSINFO_DATA(HTMLOptionsCollection,
>                            nsHTMLOptionsCollectionSH,
>                            ARRAY_SCRIPTABLE_FLAGS |
>+

Remove this.

> 
>   if (argc < 1 || !JSVAL_IS_OBJECT(argv[0])) {
>     nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
>     return JS_FALSE;
>   }
> 
>+  if (JSVAL_IS_NULL(argv[0])) {
>+    nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_INVALID_POINTER);
>+    return JS_FALSE;
>+  }
>+

Couldn't you just do something like
if (argc < 1) {...}
if (JSVAL_IS_PRIMITIVE(argv[0])) {...}
...

/me should not comment on code which he really doesn't know at all
Summary: select.options.add(null) crashes → select.options.add(null) crashes [@ JS_GetClass]
Comment on attachment 226991 [details] [diff] [review]
patch, v1

   NS_DEFINE_CLASSINFO_DATA(HTMLOptionsCollection,
                            nsHTMLOptionsCollectionSH,
                            ARRAY_SCRIPTABLE_FLAGS |
+
                            nsIXPCScriptable::WANT_SETPROPERTY)

Why add an empty line there?

   if (argc < 1 || !JSVAL_IS_OBJECT(argv[0])) {
     nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
     return JS_FALSE;
   }
 
+  if (JSVAL_IS_NULL(argv[0])) {
+    nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_INVALID_POINTER);
+    return JS_FALSE;
+  }
+

I'd say loose the !JSVAL_IS_OBJECT() check in the first if statement above and make the next one check JSVAL_IS_PRIMITIVE() and throw NS_ERROR_DOM_WRONG_TYPE_ERR if argv() is a primitive type (which null is as well as all other non-object types).

If you attach an updated patch I'll gladly r+sr...
Attachment #226991 - Flags: superreview?(jst)
Attachment #226991 - Flags: superreview-
Attachment #226991 - Flags: review?(jst)
Attachment #226991 - Flags: review-
Sorry it took me so long to post the new patch; I wanted to test it first.

smaug: I don't mind your comments one bit; I probably know less about this code than you do.
Assignee: jst → ajvincent
Status: NEW → ASSIGNED
Attachment #227145 - Flags: superreview?(jst)
Attachment #227145 - Flags: review?(jst)
Comment on attachment 227145 [details] [diff] [review]
patch answering review comments

r+sr=jst
Attachment #227145 - Flags: superreview?(jst)
Attachment #227145 - Flags: superreview+
Attachment #227145 - Flags: review?(jst)
Attachment #227145 - Flags: review+
mozilla/dom/src/base/nsDOMClassInfo.cpp 	1.396
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 227145 [details] [diff] [review]
patch answering review comments

Requesting approval for 1.8.1 and 1.8.0.x - this fixes a crash known to affect Firefox 1.5.0.4 and assertions without changing API.
Attachment #227145 - Flags: approval1.8.1?
Attachment #227145 - Flags: approval1.8.0.5?
Verified FIXED using build 2006-06-28-08 of SeaMonkey trunk on Windows XP with https://bugzilla.mozilla.org/attachment.cgi?id=226977
Status: RESOLVED → VERIFIED
Comment on attachment 227145 [details] [diff] [review]
patch answering review comments

a=darin on behalf of drivers
Attachment #227145 - Flags: approval1.8.1? → approval1.8.1+
Checking in dom/src/base/nsDOMClassInfo.cpp;
/cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v  <--  nsDOMClassInfo.cpp
new revision: 1.292.2.41; previous revision: 1.292.2.40
done

Checked into the 1.8.1 tree.
Keywords: fixed1.8.1
Comment on attachment 227145 [details] [diff] [review]
patch answering review comments

approved for 1.8.0 branch, a=jay for drivers.  please land asap, so we can respin and get RC2 out for testing.
Attachment #227145 - Flags: approval1.8.0.5? → approval1.8.0.5+
mozilla/dom/src/base/nsDOMClassInfo.cpp 	1.292.2.18.2.8
Keywords: fixed1.8.0.5
*** Bug 343350 has been marked as a duplicate of this bug. ***
*** Bug 343362 has been marked as a duplicate of this bug. ***
Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.5) Gecko/20060705 Firefox/1.5.0.5.
Crash Signature: [@ JS_GetClass]
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: