Closed Bug 686274 Opened 9 years ago Closed 7 years ago

"array initialiser too large" when initializing JavaScript array with 660k entries

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: eliasen, Assigned: luke)

References

Details

Attachments

(2 files)

Attached file bigarray.html.gz
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0.2) Gecko/20100101 Firefox/6.0.2
Build ID: 20110906120836

Steps to reproduce:

A large JavaScript array initializer with the format 

var words = ["$100-a-plate dinner",
"1080",
"10 Downing Street",
"10-point",
"10th",
"11-point",
"1200 hours",
...
];

fails with an error message "array initialiser too large".  


This occurs in Firefox 6.0.2, Mozilla/5.0 (X11; Linux x86_64; rv:6.0.2) Gecko/20100101 Firefox/6.0.2


Actual results:

The array has 616671 entries (each on its own line.)  The error message occurs on or after line 523264 of the file, which corresponds to the 523264th entry in the array.  The position in the array that it fails on is apparently constant whatever the data is in the array.  The wordlist was replaced with 

var nums = [1,
2,
3,
4,
5,
6,
...

and the error still occurred on the same line number.


Expected results:

The array should have been created.  Artificial limits in parsing should be treated as a bug.

It appears that the same/similar bug was patched for JSON in Bug 667527, but the bug still occurs in the core language, and the patch may have been put into the wrong place to fix the general problem.
Attached testcase is an HTML file with embedded JavaScript that demonstrates the program.   You should see an alert box if JavaScript is working correctly.  If it's not working correctly, check the error console to see what line that the JavaScript parser broke on.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Is the "evil case" in js/src/jsarray.cpp NewbornArrayPushImpl related to this bug and to "sharp variables" and the supposedly-now-fixed secret Bug 630377? 

Unfortunately, I can't view Bug 630377 to know if this can be fixed yet.
Bug 630377 doesn't seem to be related.

The issue is really that in jsparse we have this code for array literals:

            for (index = 0; ; index++) {
                if (index == StackSpace::ARGS_LENGTH_MAX) {
                    reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_ARRAY_INIT_TOO_BIG);
                    return NULL;
                }

ARGS_LENGTH_MAX is defined as:

    static const uintN ARGS_LENGTH_MAX = CAPACITY_VALS - (2 * BUFFER_VALS);

where:

    static const size_t CAPACITY_VALS  = 512 * 1024;
    static const size_t BUFFER_VALS    = 16 * 1024;

So ARGS_LENGTH_MAX is 480*1024 = 491520.

Which is of course less than the number of entries in this array (616671).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: "array initialiser too large" when initializing JavaScript array → "array initialiser too large" when initializing JavaScript array with 660k entries
Version: 6 Branch → Trunk
Would it make sense to nudge the limit up a bit? I've been hitting this at work, and it seems somewhat restrictive. Perhaps there are good technical reasons for this limit, but my charts so far seem to work plenty fast when filled with 400k arrays.
Hah, the limit is artificially capped at StackSpace::ARGS_LENGTH_MAX (before that, JS_ARGS_LENGTH_MAX), which is the maximum number of actual arguments that can be passed in a function call (limited by the size of the JS stack).  However, this was because array initializers used to push all the elements on the stack before calling a function to construct the array.  We have since stopped doing that, so this could be relaxed to JSObject::NELEMENTS_LENGTH, which is 2^28.

The fix is easy, just need to write generate a few large testcases.
Attached patch fix and testSplinter Review
With this patch, the limit is 2^28.  I tried to write a test for >= 2^28, but it takes way too long to complete (at least in debug builds).  I did add a test, however, that 1M elements passes.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #751813 - Flags: review?(bhackett1024)
Hah, I just noticed that bz said the same thing in comment 3.
Well, I said the "capped" part, not the very important "no longer needs to be capped" part... ;)
Attachment #751813 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/1719a495c7c2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 1199345
You need to log in before you can comment on or make changes to this bug.