394941 - Infinite recursion causes "out of memory" instead of catchable "stack overflow" in this case

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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?

Comment 1

[Igor Bukanov]

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.

Comment 2

[Igor Bukanov]

Attachment: fix v1

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 3

[Brendan Eich [:brendan]]

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

Comment 4

[Brendan Eich [:brendan]]

(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 5

[Brendan Eich [:brendan]]

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

Comment 6

[Igor Bukanov]

(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.

Comment 7

[Igor Bukanov]

(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.

Comment 8

[Igor Bukanov]

Attachment: fix v2

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.

Comment 9

[Igor Bukanov]

Attachment: v1-v2 diff

A plain diff between v1 and v2.

Comment 10

[Igor Bukanov]

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.

Comment 11

[Igor Bukanov]

Attachment: fix for e4x/XML/regress-324422-1.js

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

Comment 12

[Igor Bukanov]

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 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 289459 [details] [diff] [review]
fix v2

a=beltzner for drivers

Comment 14

[Igor Bukanov]

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

Comment 15

[Bob Clary [:bc] (inactive)]

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

Comment 16

[Bob Clary [:bc] (inactive)]

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

Comment 17

[Igor Bukanov]

(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.

Comment 18

[Bob Clary [:bc] (inactive)]

(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.

Comment 19

[Igor Bukanov]

(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.

Comment 20

[Bob Clary [:bc] (inactive)]

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

Comment 21

[Bob Clary [:bc] (inactive)]

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

Comment 22

[Bob Clary [:bc] (inactive)]

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