350256 - apply can't accept parameter arrays larger than 65534

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bertrand Le Roy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

One of the fastest ways to clone an array is to apply the Array constructor to it this way: Array.apply(null, myArray).
Unfortunately, this fails in Firefox with large sparse arrays due to some limitiation on the number of parameters that apply accepts. It may be a problem in the number of parameters any function accepts but that's harder to test from my end.

Reproducible: Always

Steps to Reproduce:
var a = new Array();
a[65534] = 'yes'
a[65535] = 'no';

var b = Array.apply(null, a);

alert(b.length + "," + b[65534] + "," + b[65535]);

function f() {
    alert(arguments.length + "," + arguments[65534] + "," + arguments[65535]);
}

f.apply(null, a);

Actual Results:  
65536,yes,no
65536,yes,no

(that's what IE is returning)

Expected Results:  
65535,yes,undefined
65535,yes,undefined

One strange thing is the size of the array, which is off by one, but if you try the same thing without setting the 65534 element, the length is still 65535 so it's not just the 65535 element being ignored.

Comment 1

[Brendan Eich [:brendan]]

Yes, we limit actual argument count to 65535 for Function.prototype.apply, since that is the maximum number of actuals you can push and pass in an explicit call to any function.

We could raise this limit, while still imposing an upper bound for sanity and to avoid trivial DOS attacks.  What does IE do?  (The "Expected" and "Actual" results in comment 0 seem to have their titles transposed.)

/be

Comment 2

[Bertrand Le Roy]

(In reply to comment #1)
> Yes, we limit actual argument count to 65535 for Function.prototype.apply,
> since that is the maximum number of actuals you can push and pass in an
> explicit call to any function.
> We could raise this limit, while still imposing an upper bound for sanity and
> to avoid trivial DOS attacks.  What does IE do?  (The "Expected" and "Actual"
> results in comment 0 seem to have their titles transposed.)
> /be

Yes, absolutely, expected and actual were reversed, sorry about that. IE does the expected thing here. I don't know if they have a limit on the number of parameters. I can ask them if that helps.

I think the right thing to do here is to limit the number of actually defined elements, not the length of the array.

Comment 3

[Bertrand Le Roy]

One more thing to note is that it's not just IE that gives the right result here. So do Opera and Safari.

Comment 4

[Brendan Eich [:brendan]]

We can increase the limit to 2^24 (max length 2^24 - 1).  Brian, can you field this one for the trunk, and we can consider for js1.7src/mozilla1.8.1.1?  Thanks,

/be

Comment 5

[Brian Crowder]

Attachment: decoupled ARGC_LIMIT from jsfun

Now using ARRAY_INIT_LIMIT from jsparse.c which I have moved to jsarray.h (now also included by jsparse.c -- is that new compilation coupling okay?) so this 24-bit size is shared between both.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 242804 [details] [diff] [review]
decoupled ARGC_LIMIT from jsfun


> #include "jstypes.h"
> #include "jsarena.h" /* Added by JSIFY */
>+#include "jsarray.h"
> #include "jsutil.h" /* Added by JSIFY */
> #include "jsapi.h"

Nit: #include "jsarray.h" here, in alphabetical order after jsutil.h, which is the dividing line between the "NSPR-forked" and "core SpiderMonkey" headers.

Yeah, it's ok to include jsarray.h in jsparse.c.

/be

Comment 7

[Brian Crowder]

Attachment: nit picked

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 242808 [details] [diff] [review]
nit picked

With this patch, does the assertion at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsstr.c&rev=3.134&root=/cvsroot&mark=2304#2304 fire if you call "".fromCharCode.apply with an array that's larger than ARGC_LIMIT?

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (From update of attachment 242808 [details] [diff] [review] [edit])
> With this patch, does the assertion at
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsstr.c&rev=3.134&root=/cvsroot&mark=2304#2304
> fire if you call "".fromCharCode.apply with an array that's larger than
> ARGC_LIMIT?

Good catch!  Brian, fix that to use ARRAY_INIT_LIMIT and I'll gladly r+.

/be

Comment 10

[Brian Crowder]

Attachment: nice find, mrbkap

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 242831 [details] [diff] [review]
nice find, mrbkap

r=me, thanks.  I'll check this in shortly.

/be

Comment 12

[Brendan Eich [:brendan]]

Fix landed on trunk:

Checking in jsarray.h;
/cvsroot/mozilla/js/src/jsarray.h,v  <--  jsarray.h
new revision: 3.15; previous revision: 3.14
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.169; previous revision: 3.168
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.259; previous revision: 3.258
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.135; previous revision: 3.134
done

/be

Comment 13

[Bob Clary [:bc] (inactive)]

Checking in slow-n.tests;
/cvsroot/mozilla/js/tests/slow-n.tests,v  <--  slow-n.tests
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Array/regress-350256-01.js,v
done
Checking in js1_5/Array/regress-350256-01.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-350256-01.js,v  <--  regress-350256-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Array/regress-350256-02.js,v
done
Checking in js1_5/Array/regress-350256-02.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-350256-02.js,v  <--  regress-350256-02.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Array/regress-350256-03.js,v
done
Checking in js1_5/Array/regress-350256-03.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-350256-03.js,v  <--  regress-350256-03.js
initial revision: 1.1
done

added 03 to slow-n.tests. Note that IE6 passes for length 2^24 but the max Spidermonkey can handle is 2^24-1.

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061028 windows/linux

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 242831 [details] [diff] [review]
nice find, mrbkap

approved for 1.8 branch, a=dveditz for drivers

Comment 16

[:Gavin Sharp [email: [e-mail]]]

mozilla/js/src/jsarray.h 3.10.22.3
mozilla/js/src/jsfun.c 3.117.2.28
mozilla/js/src/jsparse.c 3.142.2.70
mozilla/js/src/jsstr.c 3.108.2.12

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 20061122 1.8.1.1 windows/linux/mac, 1.9 windows/linux

note to self: -03 appeared to crash for trunk debug browser on the win2k3 server machines but I could not reproduce locally on windows xp.  1.8.1.1 debug browser timed out on -03 on win2k3 server. On winxp 1.8.1.1 opt browser didn't shut down cleanly but debug did.