Last Comment Bug 607371 - Greater-than-maximum argument count handling is prone to silent errors
: Greater-than-maximum argument count handling is prone to silent errors
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 568663 578705 586525
  Show dependency treegraph
Reported: 2010-10-26 10:39 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-08-01 11:10 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch, with various test updates (7.52 KB, patch)
2011-07-25 17:41 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Er, the real patch (4.35 KB, patch)
2011-07-25 17:42 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2010-10-26 10:39:06 PDT
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] (remove +bmo to email) 2011-07-23 12:01:08 PDT
Bug 568663 can also be blamed on this bug.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-25 17:41:25 PDT
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.)
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-25 17:42:11 PDT
Created attachment 548344 [details] [diff] [review]
Er, the real patch
Comment 4 Luke Wagner [:luke] 2011-07-25 17:59:58 PDT
Comment on attachment 548344 [details] [diff] [review]
Er, the real patch

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


::: 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] (remove +bmo to email) 2011-07-29 17:33:32 PDT

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] 2011-08-01 08:00:00 PDT

Note You need to log in before you can comment on or make changes to this bug.