60920 - javascript crash when working with arrays and new Option();

Status: RESOLVED FIXED

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

Description

[Morgan Christiansson]

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 1

[gstoll]

Confirmed on Linux build 2000112108.  The url given crashes Mozilla.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Phil Schwartau]

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 -

Comment 4

[Phil Schwartau]

Just noticed this, too: there is no closing <FORM> tag. Does that matter? 

Comment 5

[Boris Zbarsky [:bzbarsky]]

I added the closing </form> and </html> tags when I tested this...  I should
have mentioned that.  The crash still occurs.

Comment 6

[Morgan Christiansson]

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.

Comment 7

[Johnny Stenback (:jst)]

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...

Comment 8

[Johnny Stenback (:jst)]

Attachment: testcase

Comment 9

[Johnny Stenback (:jst)]

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...

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Johnny Stenback (:jst)]

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?

Comment 12

[Johnny Stenback (:jst)]

Attachment: Propsed fix (cvs diff -w)

Comment 13

[Johnny Stenback (:jst)]

Brendan, could you have a look at the attached patch? sr=? :-)

Comment 14

[Brendan Eich [:brendan]]

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=[e-mail].

/be

Comment 15

[Johnny Stenback (:jst)]

Attachment: New proposed fix

Comment 16

[Johnny Stenback (:jst)]

Never mind the last patch, new patch is coming up...

Comment 17

[Johnny Stenback (:jst)]

Attachment: Yet one proposed fix, this one is going in!

Comment 18

[Brendan Eich [:brendan]]

Looks good, but what about those extra newlines?

+              if (parent) {

+

+                parent->ReplaceChild(option, refChild, getter_AddRefs(ret));

+              }


/be

Comment 19

[Johnny Stenback (:jst)]

Fix checked in and I removed the extra newlines that accidentally ended up in
the diff. Thanks for the reviews!

Marking FIXED.