Steps to reproduce: paste this in to ./js (debug) : try { function f() { var z = <x><y/></x>; f(); } f(); } catch(e) { print("Caught: " + e) } Result: "out of memory" Expected: "Caught: InternalError: stack overflow in f" Changing the argument from that E4X object to the number 1 makes it give "stack overflow" as desired. Related to bug 393368?

The reason for the bug is that var z = <x><y/></x> translates into en equivalent of z = XML("<x><y/></x>"). That at runtime uses the arena allocation to parse XML tree. With the bug 393368 fixed the allocation is bounded by the same stack quota as JS call recursion. The problem is that XML parser still uses js_ReportOutOfMemory instead of throwing an appropriate error. Hence OOM would be triggered if during the recursion it is the parser that hits the quota.

The patch introduces JSMSG_SCRIPT_STACK_QUOTA and a convenience function js_ReportOutOfScriptQuota to report errors when quota-bounded arena allocation fails. The function is used when the quota is exhausted outside the compiler. In the compilation code the patch uses js_ReportCompileErrorNumber. Since that needs one of JSCodeGenerator*, JSParseNode* or JSTokenStream* to report file name and line number, I changed few functions in jsparse.c to accept an extra JSTokenStream* parameter when neither the code generator or a parse node is not available. For symmetry I also added js_ReportOverRecursed(JSContext *cx) as a shortcut for JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_OVER_RECURSED); The drawback of the patch is that a real out-of-memory in jsarena is still reported as out-of-script-quota exception. But in that case the subsequent memory allocations will also fail and OOM will be eventually reported. Another possibly controversial feature of ther patch is that it removes JSMSG_STACK_OVERFLOW and uses js_ReportOutOfScriptQuota to report AllocRawStack errors. Due to generic nature of JSMSG_SCRIPT_STACK_QUOTA it does not include the function name into the error message. That means that infinite recursion errors would not include the name either. But that information is still recoverable from either the stack or from the source line info properties of exception objects.

Comment on attachment 281455 [details] [diff] [review] fix v1 s/ScriptQuota/StackQuota/g Could you put the tc into the pc and avoid that extra arg all over? /be

(In reply to comment #3) > (From update of attachment 281455 [details] [diff] [review]) > Could you put the tc into the pc and avoid that extra arg all over? Sorry, s/tc/ts/ /be

Comment on attachment 281455 [details] [diff] [review] fix v1 Or if you prefer, combine "context params" in a new bug. It seems to me we could cut down on actual argument memory traffic by folding more into JSParseContext. cx and ts and tc all might fit into pc. What do you think? /be

(In reply to comment #5) > (From update of attachment 281455 [details] [diff] [review]) > Or if you prefer, combine "context params" in a new bug. It seems to me we > could cut down on actual argument memory traffic by folding more into > JSParseContext. cx and ts and tc all might fit into pc. Yesterday I realized that it is better just remove JSParseContext and move all its fields to JSTokenStream. The fields It simplifies a lot of things including the removal of pn_ts field frin JSParseNode. But it is going to be a big patch so I will try to split it to manageable parts.

(In reply to comment #6) The previous comments suffered from an accidental commit, so here is comprehensible text: Yesterday I realized that it is better just remove JSParseContext and move all its fields to JSTokenStream. It simplifies a lot of things including the removal of pn_ts field from JSParseNode. But it is going to be a big patch so I will try to split it to manageable parts and file separated bugs.

In the new version I always use js_ReportOutOfScriptQuota and js_ReportOverRecursed even in the parser/compiler where the previous version has used ReportCompileErrorNumber for simpler code. In general a stack overflow or hitting stack quota do not correlate with a place in a overly complex script that would trigger them. Thus including the line number into the error reports would be misleading and a general runtime report should be enough.

A plain diff between v1 and v2.

The patch from comment 9 triggers expected regression in e4x/XML/regress-324422-1.js as the test cases assumes that parsing too complex xml would trigger out-of-memory leading to the exit code 5 when the shell exits. With the patch that error becomes an ordinary runtime exception with 3 as the exit code.

The patch uses try/catch to capture error about exhausted script quota.

Comment on attachment 289459 [details] [diff] [review] fix v2 The patch fixes an error handling regression that leads to bogus out-of-memory errors.

Comment on attachment 289459 [details] [diff] [review] fix v2 a=beltzner for drivers

I checked in the patch from comment 8 to the trunk of CVS: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1195769963&maxdate=1195770240&who=igor%mir2.org

Checking in e4x/XML/regress-324422-1.js; /cvsroot/mozilla/js/tests/e4x/XML/regress-324422-1.js,v <-- regress-324422-1.js new revision: 1.6; previous revision: 1.5 done Checking in js1_5/Function/regress-338121-03.js; /cvsroot/mozilla/js/tests/js1_5/Function/regress-338121-03.js,v <-- regress-338121-03.js new revision: 1.3; previous revision: 1.2 done RCS file: /cvsroot/mozilla/js/tests/e4x/Regress/regress-394941.js,v done Checking in e4x/Regress/regress-394941.js; /cvsroot/mozilla/js/tests/e4x/Regress/regress-394941.js,v <-- regress-394941.js initial revision: 1.1 done

Checking in public-failures.txt; /cvsroot/mozilla/js/tests/public-failures.txt,v <-- public-failures.txt new revision: 1.6; previous revision: 1.5 done

(In reply to comment #15) > Checking in e4x/XML/regress-324422-1.js; > /cvsroot/mozilla/js/tests/e4x/XML/regress-324422-1.js,v <-- > regress-324422-1.js The test e4x/XML/regress-324422-1.js still contains: if (typeof document == 'undefined') { printStatus ("Expect possible out of memory error"); expectExitCode(0); expectExitCode(5); } This is no longer necessary since the purpose of the script quota was to avoid out-of-memory via too complex scripts.

(In reply to comment #17) > The test e4x/XML/regress-324422-1.js still contains: ... Yep, because I have to keep running this on 1.8 for the foreseeable future.

(In reply to comment #18) > Yep, because I have to keep running this on 1.8 for the foreseeable future. This suggests to add a method to js shell to allow to distinguish between branches and use it for conditional code. Something like engineVersion() returning a string in a predefined format or even a global const like ENGINE_VERSION.

I've often wanted something like that in the shell since I could, if needed, get the same information in the browser.

I modified the test to use regexps to allow for the difference between 1.8 and 1.9.0. Checking in regress-394941.js; /cvsroot/mozilla/js/tests/e4x/Regress/regress-394941.js,v <-- regress-394941.js new revision: 1.2; previous revision: 1.1

verified fixed 2007-12-09 1.9.0 linux|mac|win