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

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Bug 568663 can also be blamed on this bug.

Updated

6 years ago
Blocks: 586525, 568663
Created attachment 548343 [details] [diff] [review]
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.)
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #548343 - Flags: review?(luke)
Created attachment 548344 [details] [diff] [review]
Er, the real patch
Attachment #548343 - Attachment is obsolete: true
Attachment #548344 - Flags: review?(luke)
Attachment #548343 - Flags: review?(luke)

Comment 4

6 years ago
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.
Attachment #548344 - Flags: review?(luke) → review+
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.
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/50895f8c1f50
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 578705
You need to log in before you can comment on or make changes to this bug.