607371 - Greater-than-maximum argument count handling is prone to silent errors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Bug 586525 occurs because when we call a function in JS, we call it with up to JS_ARGS_LENGTH_MAX arguments -- and we silently discard any arguments beyond that limit.  In this case we'd have diagnosed the problem much more readily if an exception had been thrown.  So maybe we should throw an exception if you call a function with more than the maximum-argument count, or warn, or something else, dunno what -- but in any case, completely silent failure doesn't seem like something people wishing to write correct code will find acceptable.

Comment 1

[Jeff Walden [:Waldo]]

Bug 568663 can also be blamed on this bug.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch, with various test updates

This implements v8's behavior (see also bug 578705, which is basically a dup of this, or the other way around), modulo differences in the exact number of arguments that trigger the exception in each engine.  (It looks like they might trigger based on overall stack size [at least if the error message is any guide], and their max limit is a bit smaller than our.)

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Er, the real patch

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 548344 [details] [diff] [review]
Er, the real patch

Review of attachment 548344 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/jit-test/tests/basic/testBug653396.js
@@ +2,3 @@
>  function g(a, b, c, d) {}
>  function f(a, b, c) {
> +        arguments.length = 512 * 1024 - 2 * 16 * 1024 + 1; // StackSpace::ARGS_LENGTH_MAX + 1

There is a new shell function in town: getMaxArgs().  How about you use it here.

Comment 5

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/50895f8c1f50

Past using getMaxArgs(), I also remembered to change the other half of splatApplyArguments (or however it's named) for the other way that the apply optimization had to be changed, and I added a test that hit that other quirky code and would fail without this patch.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/50895f8c1f50