667527 - array initialiser too large

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mischa]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/534.30 (KHTML, like Gecko) Chrome/12.0.742.100 Safari/534.30

Steps to reproduce:

I receive a big json object containing a very large array.


var json_res = '{"value":"I am big JSON object"}';
var o = eval('(' + json_res + ')');



Actual results:

Code throws an Internal exception with message "array initialiser too large"


Expected results:

JavaScript eval function show create a nice object as it did in Firefox 4!!

Comment 1

[Boris Zbarsky [:bzbarsky]]

A testcase would be useful here....

Comment 2

[David Mandelin [:dmandelin]]

(In reply to comment #1)
> A testcase would be useful here....

And also a regression window along with the test case.

Comment 3

[Mischa]

Attachment: testcase

Comment 4

[Mischa]

(In reply to comment #2)
> (In reply to comment #1)
> > A testcase would be useful here....
> 
> And also a regression window along with the test case.

I don't know what you mean by regression window, but I think the the attachment will be sufficient. 

Small How-TO:
1. Load file and execute the script
2. Get a "No bug", if json object was created successfully
or get an error message via window.alert .

Comment 5

[Jeff Walden [:Waldo]]

(gdb) bt 8
#0  ReportError (cx=0xac6e80, message=0xac8750 "array initialiser too large", reportp=0x7fffffffbc00, 
    callback=0x45d036 <js_GetErrorMessage(void*, char const*, uintN)>, userRef=0x0) at /home/jwalden/moz/shell-js/js/src/jscntxt.cpp:607
#1  0x000000000045ca44 in js_ReportErrorNumberVA(JSContext *, uintN, JSErrorCallback, void *, uintN, JSBool, typedef __va_list_tag __va_list_tag *) (cx=0xac6e80, 
    flags=0, callback=0x45d036 <js_GetErrorMessage(void*, char const*, uintN)>, userRef=0x0, errorNumber=200, charArgs=0, ap=0x7fffffffbcb0)
    at /home/jwalden/moz/shell-js/js/src/jscntxt.cpp:968
#2  0x00000000004305da in JS_ReportErrorNumberUC (cx=0xac6e80, errorCallback=0x45d036 <js_GetErrorMessage(void*, char const*, uintN)>, userRef=0x0, errorNumber=200)
    at /home/jwalden/moz/shell-js/js/src/jsapi.cpp:5724
#3  0x000000000044cd8c in ArrayCompPushImpl (cx=0xac6e80, obj=0x7ffff1b0f120, v=...) at /home/jwalden/moz/shell-js/js/src/jsarray.cpp:2074
#4  0x000000000044658d in js_ArrayCompPush (cx=0xac6e80, obj=0x7ffff1b0f120, vp=...) at /home/jwalden/moz/shell-js/js/src/jsarray.cpp:2093
#5  0x00000000004f8e36 in JSONParser::parse (this=0x7fffffffc0c0, vp=0x7fffffffc140) at /home/jwalden/moz/shell-js/js/src/jsonparser.cpp:587
#6  0x00000000004da2c4 in EvalKernel (cx=0xac6e80, call=..., evalType=DIRECT_EVAL, caller=0x7ffff1ce0030, scopeobj=...)
    at /home/jwalden/moz/shell-js/js/src/jsobj.cpp:1208
#7  0x00000000004da7da in js::DirectEval (cx=0xac6e80, call=...) at /home/jwalden/moz/shell-js/js/src/jsobj.cpp:1305

Looks to me like we should rename js_ArrayCompPush, remove its argument-length restriction (should that even be there for array comprehensions?), and declare victory.

All that aside, Mischa, you really should use JSON.parse rather than eval to parse JSON data.  It's faster and safer than eval, and it means you don't have to surround the string in parentheses before parsing (which in SpiderMonkey at the moment will double the amount of memory needed to store the string).

Comment 6

[Mischa]

Thanks for this tip, but JSON.parse gives me the same error.

As I was working on some workarounds I found the same problem if I just have a very big string no matter if I use JSON.parse or window.eval.

Any other suggestions?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Thanks for this tip, but JSON.parse gives me the same error.

Right, because we actually have a workaround to detect that you're trying to use eval to parse JSON and call the same code internally instead of doing an actual eval...  Jeff's suggestion was separate from this issue, which is a bug in our code; just a general "how to parse JSON faster and with less memory usage" suggestion.  ;)

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Patch and tests

This could really be about a two-line patch to remove the |length > JS_ARGS_LENGTH_MAX|, but the method names have been misleading for quite a while and are in need of adjustment.  I'll whip up that two-liner for the relevant branches, as it seems like we want to backport this, and the naming doesn't much matter for non-trunk.

Hopefully we can kill the "evil case" comment soon.  The use that necessitates it is really a violation of the newly-codified js_NewbornArrayPush interface, but right now I don't have much interest in finding and doing something else.

Comment 9

[Jeff Walden [:Waldo]]

Regarding using JSON.parse over eval and comment 7, I'd add a few extra details.

First, the optimization of eval for JSON strings is only a guess.  If the string passes our mostly-quick looks-slightly-like-JSON tests, we still have to actually parse to determine whether it is indeed JSON.  If we're wrong, it's going to cost more to use eval than it really should, because we'll do a bunch of work for nothing.  So the workaround speeds up some code that tries to eval JSON, and it slows down some code that tries to eval, to actually eval.  (Ideally at some point we'll remove this optimization, but that's probably a ways off yet.)

Second, using eval over JSON.parse entails extra overhead because it looks like you'll be doing an eval.  We can execute methods that don't appear to call eval much faster than we can execute methods which appear as if they might, because eval disables various optimizations:

http://whereswalden.com/2011/01/10/new-es5-strict-mode-support-new-vars-created-by-strict-mode-eval-code-are-local-to-that-code-only/

Third, if you happen to have a site which worries about XSS and the like, you might want to use CSP to tell the browser to guard against exploitation of holes you don't even know about:

http://engineering.twitter.com/2011/03/improving-browser-security-with-csp.html

If you use CSP, you almost certainly will disable code execution from strings, which deliberately breaks eval.  You *have* to use JSON.parse if you use CSP.

Fourth, we disable the eval-as-JSON hack for strict mode code, so if you ever start using strict mode, eval of JSON will be much slower.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Aurora patch (applies to mozilla-beta with a little jstests.list fuzzing)

Comment 11

[David Mandelin [:dmandelin]]

Comment on attachment 542511 [details] [diff] [review]
Patch and tests

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

Comment 12

[David Mandelin [:dmandelin]]

Comment on attachment 542572 [details] [diff] [review]
Aurora patch (applies to mozilla-beta with a little jstests.list fuzzing)

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

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 542572 [details] [diff] [review]
Aurora patch (applies to mozilla-beta with a little jstests.list fuzzing)

This makes it possible to parse JSON containing arrays with 524288 or more elements, and to evaluate array comprehensions which generate arrays containing more than that many elements.  Someone's run into the former, and the fix is simple and straightforward, so we should fix it.

Comment 14

[Johnathan Nightingale [:johnath]]

Comment on attachment 542572 [details] [diff] [review]
Aurora patch (applies to mozilla-beta with a little jstests.list fuzzing)

please get it into trunk and aurora right quick - migration is next week!

Comment 15

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/bc1e401e5bb5
http://hg.mozilla.org/releases/mozilla-beta/rev/23cc894f703c
http://hg.mozilla.org/releases/mozilla-aurora/rev/da6f57193888

Comment 16

[Jeff Walden [:Waldo]]

Erm.  I totally misread the subject of bugmail and thought it said a-m-b+ when it said exactly the opposite!  :-(  (Possibly my eyes seized on the "granted" in both messages, if so a casualty of the "denied" bikeshed.)  Should I back out the m-b changeset noted in comment 15?

Comment 17

[Johnathan Nightingale [:johnath]]

The beta landing is moot - that's the beta repo for Firefox 5, the migration of Firefox 6 to that channel happens today. You should ping legneato to see if he wants you backed out to simplify the migration work, but the aurora landing in comment 15 means it'll be in FF6 beta anyhow.

(And thanks for owning the mistake and asking about it instead of just letting it sit)

Comment 18

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/bc1e401e5bb5

Comment 19

[Asa Dotzler [:asa]]

This seems to have made the uplift to Firefox 6 Beta. Is there any reason to continue tracking this?

Comment 20

[Remus Pop (:RemusPop)]

Verified issue using the test case attached on the following configurations:
Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Setting status to VERIFIED FIXED.

Comment 21

[Alan Eliasen]

Bug 686274 seems to be another manifestation of this bug.