Closed Bug 350256 Opened 14 years ago Closed 13 years ago

apply can't accept parameter arrays larger than 65534

Categories

(Core :: JavaScript Engine, defect)

1.8 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: bleroy, Assigned: crowderbt)

References

Details

(Keywords: verified1.8.1.1)

Attachments

(1 file, 2 obsolete files)

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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
One more thing to note is that it's not just IE that gives the right result here. So do Opera and Safari.
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
Assignee: general → crowder
Attached patch decoupled ARGC_LIMIT from jsfun (obsolete) — Splinter Review
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.
Attachment #242804 - Flags: review?(brendan)
Status: NEW → ASSIGNED
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
Attached patch nit picked (obsolete) — Splinter Review
Attachment #242804 - Attachment is obsolete: true
Attachment #242808 - Flags: review?(brendan)
Attachment #242804 - Flags: review?(brendan)
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?
(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
Attachment #242808 - Attachment is obsolete: true
Attachment #242831 - Flags: review?(brendan)
Attachment #242808 - Flags: review?(brendan)
Comment on attachment 242831 [details] [diff] [review]
nice find, mrbkap

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

/be
Attachment #242831 - Flags: review?(brendan)
Attachment #242831 - Flags: review+
Attachment #242831 - Flags: approval1.8.1.1?
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
Blocks: js1.7src
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: blocking1.8.1.1?
Resolution: --- → FIXED
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.
Flags: in-testsuite+
verified fixed 1.9 20061028 windows/linux
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment on attachment 242831 [details] [diff] [review]
nice find, mrbkap

approved for 1.8 branch, a=dveditz for drivers
Attachment #242831 - Flags: approval1.8.1.1? → approval1.8.1.1+
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
Keywords: fixed1.8.1.1
OS: Windows Server 2003 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8.1
Version: Trunk → 1.8 Branch
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.
You need to log in before you can comment on or make changes to this bug.