724.76 KB, application/octet-stream
7.53 KB, patch
|Details | Diff | Splinter Review|
3.18 KB, patch
|Details | Diff | Splinter Review|
A testcase would be useful here....
(In reply to comment #1) > A testcase would be useful here.... And also a regression window along with the test case.
(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 .
(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).
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?
> 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. ;)
Created attachment 542511 [details] [diff] [review] 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.
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.
Created attachment 542572 [details] [diff] [review] Aurora patch (applies to mozilla-beta with a little jstests.list fuzzing)
Comment on attachment 542511 [details] [diff] [review] Patch and tests Review of attachment 542511 [details] [diff] [review]: -----------------------------------------------------------------
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 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 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!
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
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?
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)
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/bc1e401e5bb5
This seems to have made the uplift to Firefox 6 Beta. Is there any reason to continue tracking this?
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.
Bug 686274 seems to be another manifestation of this bug.