393368 - js1_5/Exceptions/regress-121658.js - Out of memory

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

2007-08-21-05-trunk passes
2007-08-22-05-trunk ate my linux laptop with 1G ram and forced a power off. On a winxp box with 4G ram, it just hung the process.

bug 392973 has jelly on its face.

Comment 1

[Igor Bukanov]

The test case in question effectively checks that a recursion code like
  (function foo() { return foo(); })();
terminates with some exception.

Changes in bug 392973 removed the hard-coded limit meaning that the recursion will only terminates when malloc returns null. Since on a computer with a swap that pretty much means denial-of-service, something has to be done about that.

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Comment 3

[Igor Bukanov]

Comment on attachment 277936 [details] [diff] [review]
fix

This is OK as a fix, but I would prefer to see a limit on max size of JSContext.stackPool or even on stackPool and tempPool combined.

Comment 4

[Brendan Eich [:brendan]]

Would prefer to wait for ActionMonkey stage N, with JITting and so on. The simple API that directly controls a cheaply tested, pre-existing measure (inlineCallCount in js_Interpret) seems best for now. Thanks,

/be

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> Would prefer to wait for ActionMonkey stage N, with JITting and so on. The
> simple API that directly controls a cheaply tested, pre-existing measure
> (inlineCallCount in js_Interpret) seems best for now. 

A limit on stackPool and tempPool would allow to test otherwise untested code paths with failed arena allocations. And it is easy to implement as pool can store a pointer to the limit and update it on every malloc/free.

Comment 6

[Brendan Eich [:brendan]]

Igor, feel free to steal this, but we probably could use your help on ActionMonkey more than here. I'm with you in wanting bug 392263 fixed for 1.9, and I have a few things to do for SpiderMonkey there. Also anything to avoid merge conflicts that does not cost too much. By this reasoning, is it worth doing more here now? I am open to argument, or just bug stealing by you ;-).

/be

Comment 7

[Igor Bukanov]

Attachment: alternative script stack accounting

This is an implementation on that script stack limit accounting. 

The patch adds JSArenaPool.sizeLimit, a pointer to the limit which each malloc in arena decreases and each free increases. For all pools except  JSRuntime.propertyArenaPool the pointer is set to JSContext.scriptStackSizeLimit.

Comment 8

[Igor Bukanov]

Attachment: alternative script stack accounting v2

The new version increases JS_DEFAULT_SCRIPT_STACK_LIMIT to 32MB so ./js1_5/Regress/regress-280769-2.js does not trigger out-of-memory error when compiling scripts. Apparently the test case uses that much of memory when compiling eval's script.

Comment 9

[Igor Bukanov]

Attachment: alternative script stack accounting v3

The new version accounts for the realloc call in the arena that the previous patch missed.

Comment 10

[Igor Bukanov]

Attachment: alternative script stack accounting v4

Fixing bad accounting bug in JS_ArenaRealloc.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 278038 [details] [diff] [review]
alternative script stack accounting v4

>+JS_PUBLIC_API(void)
>+JS_SetScriptStackSizeLimit(JSContext *cx, size_t limit)
>+{
>+    cx->scriptStackSizeLimit = limit;

Suggest the following global renamings:

s/scriptStackSizeLimit/scriptStackQuota/
s/\<sizeLimit\>/quotap/ (exact word match)
s/limit/quota/ (in context of above)


>+/*
>+ * Set the fencepost (one greater than the maximum) limit on the script stack

Please lose the "fencepost (...)" language, it's wrong. s/limit/quota/

Great otherwise, r+ applies to next patch.

/be

Comment 12

[Igor Bukanov]

The patches in the bug allow to set quota on the size of stack-like structures that SpiderMonkey uses to compile and execute scripts. With a low values for quota it should help to test with the fuzzier the code paths that otherwise are taken only on real out-of-memory.  

Comment 13

[Igor Bukanov]

Attachment: alternative script stack accounting v4b

The patch with limit->quota renames.

Comment 14

[Igor Bukanov]

Attachment: alternative script stack accounting v4c

More comment text fixes.

Comment 15

[Igor Bukanov]

Attachment: alternative script stack accounting v5

The new version adds scriptStackQuota function to js shell to set/query script stack quota. Here is a usage example:

~/m/trunk/mozilla/js/src $ cat ~/m/y.js
var recusrionLevel = 0;
function f()
{
    ++recusrionLevel;
    f();
}

try {
    f();
} catch (ex) {
    print("Maximum recursion level: "+recusrionLevel);
    print("Exception: "+ex);
}
~/m/trunk/mozilla/js/src $ js -f ~/m/y.js
Maximum recursion level: 261503
Exception: InternalError: stack overflow in f
~/m/trunk/mozilla/js/src $ js -e 'scriptStackQuota(0);' -f ~/m/y.js
Maximum recursion level: 63
Exception: InternalError: stack overflow in f

Note that scriptStackQuota(0) effectively limits the script stack to the currently consumed value. This is the reason why in the above example the shell was able to compile ~/m/y.js despite zero quota which also restricts the buffers for script compilation. To trigger OOM during the compilation phase one has to use some complex script after calling scriptStackQuota(0) or use eval:

~/m/trunk/mozilla $ js -e 'scriptStackQuota(0);' -e 'eval("var x = Math.sin(0)")'
-e:1: out of memory

Here is a test for the decompiler:

~/m/trunk/mozilla $ js -e 'function f() {}; scriptStackQuota(0); ""+f;'
-e:1: out of memory

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 278228 [details] [diff] [review]
alternative script stack accounting v5

>+    JS_FN("scriptStackQuota", ScriptStackQuota, 0,0,0,0),

Would prefer stackQuota as the function name, StackQuote as the native name, just for similar brevity to surrounding shell functions.


>+JS_SetScriptStackQuota(JSContext *cx, size_t limit)
>+{
>+    cx->scriptStackQuota = limit;

s/limit/quota/g

>+    jsuword boff, aoff, extra, hdrsz, gross, grossExtra;

Suggest s/grossExtra/growth/g.

r+a=me with these picked.

/be

Comment 17

[Igor Bukanov]

Attachment: alternative script stack accounting v5b

The new version addresses the nits and renames scriptStackQuota->stackQuota:

js> eval("1+1")
2
js> stackQuota(0)
js> eval("1+1")
eval("1+1")
typein:3: out of memory

Comment 18

[Igor Bukanov]

Attachment: alternative script stack accounting v5c

The new version fixes bad indentation in the previous one.

Comment 19

[Igor Bukanov]

I checked in the patch from comment 18 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1188253203&maxdate=1188253320&who=igor%mir2.org

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows