Closed
Bug 60920
Opened 24 years ago
Closed 24 years ago
javascript crash when working with arrays and new Option();
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla0.8
People
(Reporter: bugzilla.mozilla.org, Assigned: jst)
References
()
Details
(Keywords: crash, Whiteboard: [HAVE FIX])
Attachments
(4 files)
1.19 KB,
text/html
|
Details | |
1.69 KB,
patch
|
Details | Diff | Splinter Review | |
2.95 KB,
patch
|
Details | Diff | Splinter Review | |
2.96 KB,
patch
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/4.75 [en] (X11; U; Linux 2.4.0-test4 i686) BuildID: 2000101015 this happened when i was playing around with. it crashes when Engine is selected (i made it selected by default) when power is selected, it work's as it should i'll make some workaround/correction and it should work. but it's just that it shouldn't crash Reproducible: Always Steps to Reproduce: go to http://mobila.cx/mogge/research-new.html Actual Results: crash Expected Results: load the content's of the array myengine into the <SELECT NAME="r"> I don't care if it's fixed as long as it doesn't crash. a javascript error would problably be more approperiate i haven't looked much at a workaround yet so i'll submit it as normal severity.
Comment 2•24 years ago
|
||
I see this crash on linux trunk 2000-11-19-21. Confirming. I get the JavaScript errors: JavaScript strict warning: file:///home/bzbarsky/test.html line 9: assignment to undeclared variable power JavaScript strict warning: file:///home/bzbarsky/test.html line 15: assignment to undeclared variable myengine JavaScript strict warning: file:///home/bzbarsky/test.html line 21: assignment to undeclared variable i JavaScript strict warning: file:///home/bzbarsky/test.html line 22: reference to undefined property eval(newtype)[i] followed by a SIGSEGV. Stack trace: #0 JS_GetClass (cx=0x8538888, obj=0x80000000) at jsapi.c:1760 #1 0x412874dc in nsHTMLOptionCollection::SetProperty (this=0x8649330, aContext=0x8538888, aObj=0x86b0130, aID=7, aVp=0xbfffe48c) at nsHTMLSelectElement.cpp:1892 #2 0x404ca939 in nsJSUtils::nsCallJSScriptObjectSetProperty (aSupports=0x8649330, aContext=0x8538888, aObj=0x86b0130, aId=7, aReturn=0xbfffe48c) at nsJSUtils.cpp:196 #3 0x4050ffd3 in SetNSHTMLOptionCollectionProperty (cx=0x8538888, obj=0x86b0130, id=7, vp=0xbfffe48c) at nsJSNSHTMLOptionCollection.cpp:217 #4 0x401a91f0 in js_SetProperty (cx=0x8538888, obj=0x86b0130, id=7, vp=0xbfffe48c) at jsobj.c:2349 #5 0x4019c528 in js_Interpret (cx=0x8538888, result=0xbfffe520) at jsinterp.c:2475 #6 0x40193f43 in js_Invoke (cx=0x8538888, argc=0, flags=0) at jsinterp.c:807 #7 0x4019cde2 in js_Interpret (cx=0x8538888, result=0xbfffe9d4) at jsinterp.c:2603 #8 0x40193f43 in js_Invoke (cx=0x8538888, argc=1, flags=2) at jsinterp.c:807 #9 0x4019416c in js_InternalInvoke (cx=0x8538888, obj=0x84e94c0, fval=141226112, flags=0, argc=1, argv=0xbfffeb98, rval=0xbfffeb1c) at jsinterp.c:879 #10 0x4017834f in JS_CallFunctionValue (cx=0x8538888, obj=0x84e94c0, fval=141226112, argc=1, argv=0xbfffeb98, rval=0xbfffeb1c) at jsapi.c:3193 #11 0x40488199 in nsJSContext::CallEventHandler (this=0x8622df0, aTarget=0x84e94c0, aHandler=0x86af080, argc=1, argv=0xbfffeb98, aBoolResult=0xbfffeb9c, aReverseReturnResult=0) at nsJSEnvironment.cpp:915 #12 0x404ea79c in nsJSEventListener::HandleEvent (this=0x8727e48, aEvent=0x86080cc) at nsJSEventListener.cpp:154 #13 0x4115e640 in nsEventListenerManager::HandleEventSubType (this=0x872cfc0, aListenerStruct=0x827f0a8, aDOMEvent=0x86080cc, aCurrentTarget=0x8687320, aSubType=1, aPhaseFlags=7) at nsEventListenerManager.cpp:788 #14 0x4115f97c in nsEventListenerManager::HandleEvent (this=0x872cfc0, aPresContext=0x86486d8, aEvent=0xbfffefb8, aDOMEvent=0xbfffef40, aCurrentTarget=0x8687320, aFlags=7, aEventStatus=0xbfffefb0) at nsEventListenerManager.cpp:1365 #15 0x404a0239 in GlobalWindowImpl::HandleDOMEvent (this=0x8687310, aPresContext=0x86486d8, aEvent=0xbfffefb8, aDOMEvent=0xbfffef40, aFlags=1, aEventStatus=0xbfffefb0) at nsGlobalWindow.cpp:558 #16 0x414bd375 in DocumentViewerImpl::LoadComplete (this=0x8722d28, aStatus=0) at nsDocumentViewer.cpp:671 #17 0x40e1a17f in nsWebShell::OnEndDocumentLoad (this=0x868cb48, loader=0x868d8f8, channel=0x8691320, aStatus=0) at nsWebShell.cpp:954 #18 0x40e566c9 in nsDocLoaderImpl::FireOnEndDocumentLoad (this=0x868d8f8, aLoadInitiator=0x868d8f8, aDocChannel=0x8691320, aStatus=0) at nsDocLoader.cpp:815 #19 0x40e56035 in nsDocLoaderImpl::DocLoaderIsEmpty (this=0x868d8f8, aStatus=0) at nsDocLoader.cpp:621 #20 0x40e55c93 in nsDocLoaderImpl::OnStopRequest (this=0x868d8f8, aChannel=0x8595d58, aCtxt=0x0, aStatus=0, aMsg=0x0) at nsDocLoader.cpp:552 #21 0x40c56641 in nsLoadGroup::RemoveChannel (this=0x868d988, channel=0x8595d58, ctxt=0x0, aStatus=0, aStatusArg=0x0) at nsLoadGroup.cpp:576 #22 0x411db5d9 in PresShell::RemoveDummyLayoutRequest (this=0x85fcb18) at nsPresShell.cpp:5335 #23 0x411db0ec in PresShell::ReflowCommandRemoved (this=0x85fcb18, aRC=0x8595d00) at nsPresShell.cpp:5286 #24 0x411dac6b in PresShell::ProcessReflowCommands (this=0x85fcb18, aInterruptible=1) at nsPresShell.cpp:5107 #25 0x411da473 in HandlePLEvent (aEvent=0x8740220) at nsPresShell.cpp:4986 #26 0x400f1e7e in PL_HandleEvent (self=0x8740220) at plevent.c:576 #27 0x400f1d19 in PL_ProcessPendingEvents (self=0x80a5d68) at plevent.c:509 #28 0x400f3970 in nsEventQueueImpl::ProcessPendingEvents (this=0x80a5d40) at nsEventQueue.cpp:356 #29 0x406b5dbf in event_processor_callback (data=0x80a5d40, source=8, condition=GDK_INPUT_READ) at nsAppShell.cpp:158 #30 0x406b5a7d in our_gdk_io_invoke (source=0x81e1198, condition=G_IO_IN, data=0x81e1188) at nsAppShell.cpp:58 #31 0x4086faca in g_io_unix_dispatch () from /usr/lib/libglib-1.2.so.0 #32 0x40871186 in g_main_dispatch () from /usr/lib/libglib-1.2.so.0 #33 0x40871751 in g_main_iterate () from /usr/lib/libglib-1.2.so.0 #34 0x408718f1 in g_main_run () from /usr/lib/libglib-1.2.so.0 #35 0x40799c69 in gtk_main () from /usr/lib/libgtk-1.2.so.0 #36 0x406b6994 in nsAppShell::Run (this=0x80af1f8) at nsAppShell.cpp:335 #37 0x405d5fb5 in nsAppShellService::Run (this=0x80ac700) at nsAppShellService.cpp:407 #38 0x80523fb in main1 (argc=1, argv=0xbffff8b4, nativeApp=0x0) at nsAppRunner.cpp:1015 #39 0x8052d26 in main (argc=1, argv=0xbffff8b4) at nsAppRunner.cpp:1255 #40 0x403019cb in __libc_start_main (main=0x8052ba0 <main>, argc=1, argv=0xbffff8b4, init=0x804c244 <_init>, fini=0x805edcc <_fini>, rtld_fini=0x4000ae60 <_dl_fini>, stack_end=0xbffff8ac) at ../sysdeps/generic/libc-start.c:92
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•24 years ago
|
||
Here is the behavior of the given URL : IE4.7 ERROR: line 23, "Object does not support this property or method" NN4.7 ERROR: line 23 "eval has no properties" Mozilla(debug 2000-11-06 on WinNT) Loads with no errors in JavaScript console; second <SELECT>(name ="r") is not populated Mozilla(debug 2000-11-06 on Linux) CRASH on load with stack trace as above Here is the HTML of the URL: <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> <!-- X-URL: http://mog/spel/research-new.php --> <!-- Date: Wed, 22 Nov 2000 00:59:02 GMT --> <BASE HREF="http://mog/spel/research-new.php"> <HTML> <HEAD> <SCRIPT> <!-- power = new Array(new Option('Nuclear Reactor','pwr_nuke'), new Option('Fusion Power','pwr_fusion'), new Option('Solar panels','pwr_solar'), new Option('Power silo','pwr_silo')); myengine = new Array(new Option('Rocket boosters','eng_rocket'), new Option('Anti Gravity','eng_agrav'), new Option('Warp engine','eng_warp')); function switchtype() { var newtype=document.myform.type.value; for(i=0;i<power.length;i++) { document.myform.r.options[i]=eval(newtype)[i]; } } //--> </SCRIPT> </HEAD> <BODY onLoad="switchtype()"> <DIV ALIGN="center"> <H1>Allocate Research</H1> </DIV> <FORM ACTION="/spel/research-new.php" METHOD="GET" NAME="myform"> <DIV ALIGN="center"> <SELECT NAME="type" onChange="switchtype(this.value)"> <OPTION VALUE="power">Power <OPTION VALUE="myengine" SELECTED>Engine </SELECT><BR> <SELECT NAME="r" SIZE="10"> </SELECT> <INPUT TYPE="submit" VALUE="Allocate Research"> </DIV> </BODY> NOTES: 1. There is no closing HTML tag; does this matter? 2. Note the <BASE> tag is <BASE HREF="http://mog/spel/research-new.php"> Does this look right? 3. Note this above: <OPTION VALUE="myengine" SELECTED>Engine Shouldn't it say SELECTED="true" instead? 4. Note this above: var newtype=document.myform.type.value; The author expects newtype to be an array. Note that document.myform.type is a <SELECT> element. Is it correct to expect the value property of a <SELECT> to be an array? 5. Line 23 in the IE4.7 and NN4.7 error messages is the following line: document.myform.r.options[i]=eval(newtype)[i]; This corresponds to the JavaScript strict warning that Boris saw: JavaScript strict warning: file:///home/bzbarsky/test.html line 22: reference to undefined property eval(newtype)[i] Guessing this is a DOM issue rather than a JS Engine issue. Reassigning to DOM Level 0; please send back if it's JS Engine -
Assignee: rogerl → jst
Component: Javascript Engine → DOM Level 0
QA Contact: pschwartau → desale
Comment 4•24 years ago
|
||
Just noticed this, too: there is no closing <FORM> tag. Does that matter?
Keywords: crash
Comment 5•24 years ago
|
||
I added the closing </form> and </html> tags when I tested this... I should have mentioned that. The crash still occurs.
Reporter | ||
Comment 6•24 years ago
|
||
I created bug #61174 after changing the code so that the creash doesn't occur. Apparantly, neither Mozilla/Netscape 4.x likes haveing Options stored in array's. Cause Netscape 4.75/X11/Linux crashes too, however it work's a few times after that the array's become corrupt somehow and after a few more attempt's to change the category it crashes.
Assignee | ||
Comment 7•24 years ago
|
||
This crash is due to a bug on the page, the for loop tries to access past the end of the array in some cases and that's when mozilla crashes (which it shouldn't do, of course), the for loop looks like this: for(i=0;i<power.length;i++) { document.myform.r.options[i]=eval(newtype)[i]; } it does power.length iterations and accesses eval(newtype) which evaluates to the array 'myengine' whose length is shorter than the length of the array 'power'. The correct loop would be: for(i=0;i<eval(newtype).length;i++) { document.myform.r.options[i]=eval(newtype)[i]; } ... I'll see if I can figure out why mozilla crashes...
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•24 years ago
|
||
Assignee | ||
Comment 9•24 years ago
|
||
Ok, I found why mozilla crashes here, setting select[i] to undefined or any non-null and non-"object" value will crash mozilla since there's code in nsHTMLSelectElement that assumes the value is an object. This patch fixes the crash: Index: layout/html/content/src/nsHTMLSelectElement.cpp =================================================================== RCS file: /cvsroot/mozilla/layout/html/content/src/nsHTMLSelectElement.cpp,v retrieving revision 1.106 diff -u -r1.106 nsHTMLSelectElement.cpp --- nsHTMLSelectElement.cpp 2000/12/23 10:56:21 1.106 +++ nsHTMLSelectElement.cpp 2001/01/01 15:27:45 @@ -1823,7 +1823,7 @@ if ((indx >= 0) && (indx <= length)) { // if the value is null, remove this option - if (JSVAL_IS_NULL(*aVp)) { + if (!JSVAL_IS_OBJECT(*aVp)) { mSelect->Remove(indx); } else { Looking for r= and sr=, cc:ing more people...
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → mozilla0.8
Assignee | ||
Updated•24 years ago
|
Whiteboard: [HAVE FIX]
Comment 10•24 years ago
|
||
That's not the right patch, alas, because JSVAL_IS_OBJECT(JSVAL_NULL) is true -- null is considered an object according to the tag-testing macros. You could use if (JSVAL_IS_PRIMITIVE(*aVp)) ... which will remove the indexed option if the new value is not a non-null object reference, but that's a little different from what Classic did. Classic converted non-null, non-object values to object, see http://lxr.mozilla.org/classic/source/lib/libmocha/lm_input.c#1025. The logic there is tricky because you have to know that JSVAL_IS_OBJECT evaluates to true for null, so the JS_ConvertValue call won't disturb a null argument, and the JSVAL_IS_NULL test a few lines later will eval to true, leading to the removal of the desired option. I would say make the first test in the modern code use JSVAL_IS_NULL to guard the removal, then test if (JSVAL_IS_PRIMTIIVE(*aVp)) to govern a call to JS_ConvertValue, before finally checking that an object value is at hand. And of course, then check that the object's class is HTMLOptionClass or whatever it's called. /be
Assignee | ||
Comment 11•24 years ago
|
||
Brendan, I had (almost) what you suggest in my tree (I kept the null check and guarded the rest of the code with JSVAL_IS_OBJECT) but that made assigning undefined into select[x] not remove the option and I thought we'd want that to happen since that made the testcase work as expected. However, I just tested what IE 5 does and it works exactly as mozilla did with my chage, select[x] = undefined doesn't remove the option so that's probably acceptable. I'll attach a patch that uses the same approach the old code uses, is the return PR_FALSE; in the (to be attached) patch correct, will that throw an exception?
Assignee | ||
Comment 12•24 years ago
|
||
Assignee | ||
Comment 13•24 years ago
|
||
Brendan, could you have a look at the attached patch? sr=? :-)
Comment 14•24 years ago
|
||
If you don't want to test for an exact class (e.g., JS_GetClass(cx,obj) == &HTMLOptionClass), and you'd rather QI on the private data, you need to test for both JSCLASS_HAS_PRIVATE and JSCLASS_PRIVATE_IS_NSISUPPORTS (or both bits together, and clasp->flags with that, and insist you get the same two bits). Fix that and sr=brendan@mozilla.org. /be
Assignee | ||
Comment 15•24 years ago
|
||
Assignee | ||
Comment 16•24 years ago
|
||
Never mind the last patch, new patch is coming up...
Assignee | ||
Comment 17•24 years ago
|
||
Comment 18•24 years ago
|
||
Looks good, but what about those extra newlines? + if (parent) { + + parent->ReplaceChild(option, refChild, getter_AddRefs(ret)); + } /be
Assignee | ||
Comment 19•24 years ago
|
||
Fix checked in and I removed the extra newlines that accidentally ended up in the diff. Thanks for the reviews! Marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•