97921 - bad args for heavyweight function called with fewer actuals than formals

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Georg Maaß]

A valueOf to a local variable in a with block of the outer function sets the
value of that variable to zero inside the nested inner function.

A valueOf to the function reference of a nested function inside the outer
function sets the value of a local variable of the outer function to zero inside
the nested inner function.

Comment 1

[Georg Maaß]

Attachment: Test scenario

Comment 2

[Brendan Eich [:brendan]]

Need a reduced testcase.  Note also that this has nothing to do with the valueOf
method.

/be

Comment 3

[Georg Maaß]

I know that it does not have to do with the valueOf method available for
JavaScript objects. It has to do with getting the value of something like a
simple number variable, that is not a number object but a simple value, or
getting the value of a function object and may be more objects.

This testcase does *not* produce the problem, it was modified to try it on the
shell but does not produce the bug:

p=[{c:{}},{c:{}}]; doit=function(a1,a2,a3){var sptr = a2 || this;a1 = a1 || 0;	a3 =
a3 || 0;if(a3 < this.p.length){if(useWith) with(this.p[a3].c){a3;}	a3++;		this.cnt = (this.cnt||0)+1;sptr.tf =
function(){print('a3 inside nested function call:\ntype:  '+typeof a3+'\nvalue:
'+a3+' ('+this.cnt+')\nuseWith:
'+useWith+'\n');sptr.doit(a1,a2,a3);};if(this.cnt <= this.p.length)sptr.tf();
else print('Yaeh; you got the bug!\n');}else print('final a3: '+a3+'\n');}

useWith=false;cnt=0;doit();
useWith=true;cnt=0;doit();

But this one, which is not compatible with the shell, does produce the bug:


p=[{c:{}},{c:{}}];doit=function(a1,a2,a3){var sptr = a2 || this;a1 = a1 || 0;a3
= a3 || 0;if(a3 < this.p.length){if(useWith)
with(this.p[a3].c){a3;}a3++;this.cnt = (this.cnt||0)+1;sptr.tf =
function(){alert('a3 inside nested function call:\ntype:  '+typeof a3+'\nvalue:
'+a3+' ('+this.cnt+')\nuseWith:
'+useWith+'\n');sptr.doit(a1,a2,a3);};if(this.cnt <=
this.p.length)sptr.tf();else alert('Yaeh; you got the bug!\n');}else
alert('final a3: '+a3+'\n');}

useWith=false;cnt=0;doit();   // running without bug
useWith=true;cnt=0;doit();    // running with bug


No nested functions are required. It also works inside the global scope.You
can't test it on the shell, because the shell does not provide setTimeout.

This is a simplified variation of the first test scenario as displayed in the
attached file above.

Comment 4

[Phil Schwartau]

Attachment: Georg's JS shell testcase - see entry at 2001-09-01 13:21

Comment 5

[Phil Schwartau]

If I load Georg'e JS shell testcase, I DO see the bug, apparently.
Here is the JS shell output:


-----------  NOT using with statement  ------------
a3 inside nested function call:
type:  number
value:1 (1)
useWith:false

a3 inside nested function call:
type:  number
value:2 (2)
useWith:false

final a3: 2
---------------------------------------------------


-----------  USING with statement  ----------------
a3 inside nested function call:
type:  number
value:0 (1)
useWith:true

a3 inside nested function call:
type:  number
value:1 (2)
useWith:true

Yaeh; you got the bug!
---------------------------------------------------


Georg, is this what you see in the JS shell, too? 

Comment 6

[Georg Maaß]

Yes, now I see this too in the shell test case. May be that I had a miss typing
error, when I did the test yesterday on the shell, which I fixed without
knowledge when entering the test case to this bug forum.

In this test case for the shell it runs with a value, which is one step to less.
In the browser scenario, where I can use setTimeout, it is fixed to zero value.

Is it a real bug, or are my expections wrong about, what the correct behaviour
should be?

Comment 7

[Phil Schwartau]

cc'ing jband. This is the unusual testcase I was talking about -

Comment 8

[Phil Schwartau]

The testcase can be reduced to the following. I have renamed the parent
test function as 'test' and the child test function as 'test1'.

var USING_WITH = '\n-----------  USING with() statement  ----------------';
var USING_NO_WITH = '\n-----------  NOT using with() statement  ------------';
var IN_TEST = 'In test() : a3 = ';
var IN_TEST1 = 'In test1(): a3 = '
var useWith;
var a3;

print(USING_NO_WITH);
useWith=false;
test();

print(USING_WITH);
useWith=true;
test();


function test(a3)
{
  a3 = 0;  
  if(useWith) with(1){a3;}
  a3++;	

  print(IN_TEST + a3); 
  test1 = function() {print(IN_TEST1 + a3);};
  test1();
}

Comment 9

[Phil Schwartau]

The output in the SpiderMonkey shell is:

-----------  NOT using with() statement  ------------
In test() : a3 = 1
In test1(): a3 = 1

-----------  USING with() statement  ----------------
In test() : a3 = 1
In test1(): a3 = 0

Comment 10

[Phil Schwartau]

The output in the Rhino shell is:

-----------  NOT using with() statement  ------------
In test() : a3 = 1
In test1(): a3 = 1

-----------  USING with() statement  ----------------
In test() : a3 = 1
In test1(): a3 = 1

Comment 11

[Phil Schwartau]

cc'ing Brendan on this one, now that there is a reduced testcase -

Comment 12

[Phil Schwartau]

Attachment: Reduced testcase in HTML format

Comment 13

[Phil Schwartau]

The output from NN4.7 is:

----------- NOT using with() statement ------------
In test() : a3 = 1
In test1(): a3 = undefined

----------- USING with() statement ----------------
In test() : a3 = 1
In test1(): a3 = undefined

Comment 14

[Phil Schwartau]

Of course, the output from Mozilla is the same as the SpiderMonkey shell:

----------- NOT using with() statement ------------
In test() : a3 = 1
In test1(): a3 = 1

----------- USING with() statement ----------------
In test() : a3 = 1
In test1(): a3 = 0

Comment 15

[Phil Schwartau]

Here is a testcase reduced even further:

test();

function test(a3)
{ 
  a3 = 0;
  with (new Object()) {a3=100};
  a3++;	

  print(a3);
  test1 = function() {print(a3);};   
  test1();
}


OUTPUT: 

SpiderMonkey    Rhino      Mozilla      NN4.7          IE4.7
1               101        1            101            101
100             101        100          undefined      101  



I will attach the HTML version of this test below -

Comment 16

[Phil Schwartau]

Attachment: Reduced testcase #2 in HTML format

Comment 17

[Brendan Eich [:brendan]]

georg, thanks so much for finding this.  The bug is peculiar to functions that
have formal parameters, but that are called with fewer actual arguments than the
declared number of  formal parameters.  Patch coming up.

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: proposed fix: use fp->fun->nargs to bound fp->argv, not fp->argc

Comment 19

[Brendan Eich [:brendan]]

My fault, taking bug.  Please r= and sr=, all you cc: list members who care.

/be

Comment 20

[Brendan Eich [:brendan]]

Taking -- anyone try the patch yet?

/be

Comment 21

[Phil Schwartau]

I've tried the patch on WinNT; it makes both the testcases pass.
Will try the full test suite now - 

Comment 22

[Phil Schwartau]

The patch seems to be causing regressions. These tests fail on it:

           js/tests/ecma/ExecutionContexts/10.1.6.js
           js/tests/ecma/ExecutionContexts/10.1.8-1.js
           js/tests/ecma/ExecutionContexts/10.1.8-2.js
           js/tests/ecma_3/Function/arguments-001.js


For example:


Testcase ecma/ExecutionContexts/10.1.6.js failed 
Failure messages were:
     (new TestObject(0,1,2,3,4,5))[0] = undefined FAILED! expected: 0
     (new TestObject(0,1,2,3,4,5))[1] = undefined FAILED! expected: 1
     (new TestObject(0,1,2,3,4,5))[2] = undefined FAILED! expected: 2
     (new TestObject(0,1,2,3,4,5))[3] = undefined FAILED! expected: 3
     (new TestObject(0,1,2,3,4,5))[4] = undefined FAILED! expected: 4
     (new TestObject(0,1,2,3,4,5))[5] = undefined FAILED! expected: 5


Testcase ecma/ExecutionContexts/10.1.8-1.js failed 
Failure messages were:
     
GetArguments(0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25
,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,5
2,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,
79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99)[0]
     = undefined FAILED! expected: 0
     
GetArguments(0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25
,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,5
2,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,
79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99)[1]
     = undefined FAILED! expected: 1

                  etc. 
                  etc. 


Testcase ecma/ExecutionContexts/10.1.8-2.js failed 
     [ Previous Failure | Next Failure | Top of Page ]

     Failure messages were:
     
GetArguments(0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25
,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,5
2,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,
79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99)[0]
     = undefined FAILED! expected: 0
     
GetArguments(0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25
,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,5
2,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,
79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99)[1]
     = undefined FAILED! expected: 1

                  etc. 
                  etc. 



Testcase ecma_3/Function/arguments-001.js failed Bug Number 72884
     STATUS: Testing the arguments object
     Failure messages were:
     FAILED!: Section B of test
     FAILED!: Expected value '1', Actual value 'undefined'
     FAILED!: Expected value '2', Actual value 'undefined'
     FAILED!: Expected value '3', Actual value 'undefined'

Comment 23

[Phil Schwartau]

Testcase added to JS test suite:

           mozilla/js/tests/ecma_3/Function/regress-97921.js

This includes both the reduced tests above. It failed in SpiderMonkey
before I applied the above patch , but passed afterwards.

It passes in Rhino, both in compiled and interpreted mode.

Comment 24

[Brendan Eich [:brendan]]

Attachment: better fix (must use JS_MAX(fp->argc, fp->fun->nargs) to bound fp->argv accesses)

Comment 25

[Brendan Eich [:brendan]]

pschwartau, anyone: how's that latest patch for you?

/be

Comment 26

[Phil Schwartau]

The latest patch passes the testcase for this bug: 

           js/tests/ecma_3/Function/regress-97921.js

And also passes the problematic testcases above:

           js/tests/ecma/ExecutionContexts/10.1.6.js
           js/tests/ecma/ExecutionContexts/10.1.8-1.js
           js/tests/ecma/ExecutionContexts/10.1.8-2.js
           js/tests/ecma_3/Function/arguments-001.js


I'm now testing it against the rest of the test suite -

Comment 27

[Phil Schwartau]

Success - the latest patch introduces no regressions on WinNT or Linux when
run against the full JS testsuite, in both the debug and optimized JS shells.

Comment 28

[John Bandhauer]

I'm *mostly* convinced this is right. I understand most of it.

Unlike you to add the one time use variable in args_resolve :)

My brain may not be up to the task of convincing myself that you got the
args_setProperty bit right.

I'm not sure what to say... convince me?

Comment 29

[Brendan Eich [:brendan]]

You're right, that single-use variable in args_resolve is gone in my tree (I'm
getting sloppy in my old age!).

Let's look at the crucial part of args_setProperty:

-        if (0 <= argc && argc < fp->argc) {
+        if (0 <= argc && argc < fp->fun->nargs) {
             /* Check whether we need to free the deleted args bitmap. */
             if (fp->argc > JSVAL_INT_BITS &&
-                (uintN)argc <= JSVAL_INT_BITS) {
+                fp->fun->nargs <= JSVAL_INT_BITS) {
                 (void) JS_GetReservedSlot(cx, obj, 0, &bmapval);
                 if (!JSVAL_IS_VOID(bmapval)) {
                     bitmap = (jsbitmap *) JSVAL_TO_PRIVATE(bmapval);

The outer if ensures that argc < fp->fun->nargs, so the inner if needs to test
fp->fun->nargs <= JSVAL_INT_BITS to know that argc < JSVAL_INT_BITS.  The outer
if does not need to test whether argc < JS_MAX(fp->argc, fp->fun->nargs),
because if fp->argc (the old argc) is greater than fp->fun->nargs, the new argc
won't pull the JS_MAX beyond fp->fun->nargs, and the latter will determine
whether we can use a jsval as an inline jsbitmap instead of as a private-tagged
pointer to a jsbitmap vector.

IOW, we can optimize away the out-of-line jsbitmap vector only if fp->fun->nargs
(the number of formal params) is <= JSVAL_INT_BITS throughout, and the old argc
used to be > JSVAL_INT_BITS, while the new one is <= fp->fun->nargs.  Oops, that
means the outer if should test argc <= fp->fun->nargs in its 2nd clause.

New patch coming up, thanks for making me dance!

/be

Comment 30

[Brendan Eich [:brendan]]

Attachment: patch based on jband's comments

Comment 31

[John Bandhauer]

Part of what I'm thinking about is that the outer if controls whether or not
fp->argc changes or not. So if you have a case where... 

  fp->fun->nargs < argc < fp->argc

...then your logic here is not going to change fp->argc. This seems wrong. Then
again, I'm not clear on what exactly it means to set the length property of the
arguments object.

Comment 32

[Brendan Eich [:brendan]]

Setting arguments.length means setting fp->argc, because getting
arguments.length just returns fp->argc.  Now it occurs to me that we haven't
allowed one to increase argc above its last value to one <= nargs.  New patch
coming up.

/be

Comment 33

[Brendan Eich [:brendan]]

Boy, I'm batting 0 today.  Setting arguments.length should set fp->argc *not*
because the getter reflects that data member (it doesn't, due to fp->overrides),
but because fp->argc is use throughout the engine for an active frame's actual
argument count, and we want arguments.length assignment to modify that actual
argument count.  At least the GC cares -- it won't mark slots at or above index
fp->argc.

Oops, args_getProperty cares too -- it won't reflect fp->argv[slot] if slot is
not in [0, fp->argc) for the current value of fp->argc.  That seems to violate
the ECMA spec, which says that the arguments object is created on entry to a
function code execution context, and populated with indexed properties for each
actual argument (10.1.8).  The conservative thing to do is to condition all this
confusing fp->argc-reducing code with a !JSVERSION_IS_ECMA(cx->version) test, I
think.  Comments?

/be

Comment 34

[Brendan Eich [:brendan]]

Attachment: or maybe i should just nuke the non-ECMA fp->argc-mutating code in args_setProperty?

Comment 35

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I say nuke it.  I don't think anyone will miss it, and it's obviously a
maintenance headache. =)

Comment 36

[John Bandhauer]

I don't really know if anyone cares about the old behavior - I don't. I'm good 
with whatever you think is right. I'm just trying to help make sure the code 
does what you intend it to do. Is that last patch your final answer for the 
'nuke-it' strategy?

Comment 37

[Brendan Eich [:brendan]]

jband: your review is much appreciated, even better if you help me to remove
code and win weight-watchers points!  That last patch's description was a bit
confusing -- it does not remove the fp->argc-mutating code, but I'm going to
back off and nuke the site from orbit.  Net patch anon.

/be

Comment 38

[Brendan Eich [:brendan]]

Attachment: proposed fix, simplified to leave fp->argc alone when arguments.length is set

Comment 39

[Brendan Eich [:brendan]]

Ok, please r= and sr= attachment 49648 [details] [diff] [review].

/be

Comment 40

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 49648 [details] [diff] [review]
proposed fix, simplified to leave fp->argc alone when arguments.length is set

r=shaver with the removal of the unused

+    uintN minargs, maxargs;

Comment 41

[John Bandhauer]

Comment on attachment 49648 [details] [diff] [review]
proposed fix, simplified to leave fp->argc alone when arguments.length is set

you don't need...
+    uintN minargs, maxargs;

otherwise, sr=jband

Comment 42

[Brendan Eich [:brendan]]

I also don't need argc, bmapval, or bitmap -- sleepy today.  All better, thanks.

/be

Comment 43

[Phil Schwartau]

Marking Verified - 

The testcase js/tests/ecma_3/Function/regress-97921.js now passes
in the debug and optimized JS shells on WinNT, Linux, and Mac -