424683 - script stack space quota is exhausted

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

When visiting the URL the page appears to hang, and Vista HP SP1 goes 'not responding' for several seconds. 

I see in the Console2: 
Error: script stack space quota is exhausted
Source file: http://mozillalinks.org/wp/wp-content/plugins/IMM-Glossary/JavaScripts/prototype.js
Line: 584

Fallout from https://bugzilla.mozilla.org/show_bug.cgi?id=420869 ?

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Unlikely to be fallout from that bug, since that bug raised the limit and you're hitting the limit, so if anything it was just an incomplete fix for your problem -- but you don't say what version you're using, so you might be just reporting a dup of bug 420689.  What's your build ID?

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

Sorry, 

I'm using a test build from Mardak for the AwesomeBar fixups 
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008032312 Minefield/3.0b5pre Firefox/3.0 ID:2008032312 

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

Testing with the following build, I see the same.  Testing with a new Profile, no extensions I still see the hang as reported.
I also note that when the hang occurs memory use is shooting up to 250+meg, then falls back to what I consider 'normal' for my system and settings around 100-115meg.  

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008032306 Minefield/3.0b5pre Firefox/3.0 ID:2008032306

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sooooo.

In 1.8, we also hit "too much recursion", because we slam into the inline call limit of 1000.  On my MBP running trunk, by the time we've allocated too much memory, gone OOM, and therefore failed to extend the stack pool for another inline call, I show us having an inlineCallCount of around 817,000.

The regression here is that we changed from being a quick too-much-recursion error to letting the script continue until the user's machine is OOM.  This is compounded for users who have Firebug installed, because we're going to hit that error with an enormous stack and not very much memory headroom.  

(This page just has a bug, which is why it's hitting the recursion limit in 1.8, and the memory limit in 1.9.  I think we should be more robust in the face of runaway recursion, though, if we can.)

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

20:06 < shaver> right, but this is a regression from 1.8
20:06 < shaver> where we'd hit "too much recursion" without locking up the user'
s browser and consuming all their RAM
20:06 < igor> while (true) a.push(1);
20:07 < igor> shaver: this is a regression indeed.

I think we need to bring some sort of recursion limit back, given that we're finding more sites with "harmless" recursion which is now having a much greater effect on the user's machine and experience.

Perhaps we should simply restore MAX_INLINE_CALL_COUNT, since we don't have what we really want: a way to quota aggregate script memory consumption.  Apparently the recursion check was proxying for this in various content places...

An alternative might be to use some sort of non-linear operation cost accumulation for deep recursion?  I think the safest bet at this point is to restore MAX_INLINE_CALL_COUNT, possibly with a higher value - 3000 instead of 1.8's 1000?  We could put it on the context with that default, and permit embeddings to set it themselves if they want.

Comment 7

[Brian Crowder]

Attachment: restore MAX_INLINE_CALL_COUNT, removed in bug 392973

This needs more testing.

Comment 8

[Honza Bambas (:mayhemer)]

This bug is probably also fix for hang problem described in bug 303821 comment 57; and also for the crash its self.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

crowder: what testing is still needed?  I'll be lobbying for such a limit restoration this week, would love to have a ready-to-land patch if I'm successful. :)

Comment 10

[Brian Crowder]

After I wrote this, my profile manager appeared to be broken.  That was later on Friday, so I haven't come back to it yet.  I'll wrap up testing on it tonight, if no one else does.

Mainly it needs the js/tests suite and Mochis.  Booting the browser with it would be good, too.

Comment 11

[Brian Crowder]

Hm, this patch is broken.  I could have sworn I'd uploaded a newer version.  Will repair when I've finished testing.

Comment 12

[Brian Crowder]

Attachment: better

This version does "goto error" instead of "goto out".  It does not jump to bad_inline_call because newmark is not initialized yet.  Passes js/tests and while my mochitests seem to be broken it is not as a result of this.

Comment 13

[Igor Bukanov]

Comment on attachment 312663 [details] [diff] [review]
better

>Index: jsinterp.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
>retrieving revision 3.486
>diff -u -p -8 -r3.486 jsinterp.c
>--- jsinterp.c	29 Mar 2008 10:34:31 -0000	3.486
>+++ jsinterp.c	31 Mar 2008 04:08:23 -0000
>@@ -4612,16 +4612,25 @@ interrupt:
>                     uintN nframeslots, nvars, missing;
>                     JSArena *a;
>                     jsuword nbytes;
>                     void *newmark;
>                     jsval *newsp;
>                     JSInlineFrame *newifp;
>                     JSInterpreterHook hook;
> 
>+#define MAX_INLINE_CALL_COUNT 3000
>+                    /* Restrict recursion of lightweight functions. */
>+                    if (inlineCallCount == MAX_INLINE_CALL_COUNT) {
>+                        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                             JSMSG_OVER_RECURSED);
>+                        ok = JS_FALSE;
>+                        goto error;
>+                    }
>+

No need for ok = JS_FALSE as the code at the error: label sets it itself. my r+ with that fixed.


>                     /* Compute the total number of stack slots needed by fun. */
>                     nframeslots = JS_HOWMANY(sizeof(JSInlineFrame),
>                                              sizeof(jsval));
>                     nvars = fun->u.i.nvars;
>                     script = fun->u.i.script;
>                     atoms = script->atomMap.vector;
>                     nbytes = (nframeslots + nvars + script->depth) *
>                              sizeof(jsval);

Comment 14

[Igor Bukanov]

Comment on attachment 312663 [details] [diff] [review]
better

>Index: jsinterp.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
>retrieving revision 3.486
>diff -u -p -8 -r3.486 jsinterp.c
>--- jsinterp.c	29 Mar 2008 10:34:31 -0000	3.486
>+++ jsinterp.c	31 Mar 2008 04:08:23 -0000
>@@ -4612,16 +4612,25 @@ interrupt:
>                     uintN nframeslots, nvars, missing;
>                     JSArena *a;
>                     jsuword nbytes;
>                     void *newmark;
>                     jsval *newsp;
>                     JSInlineFrame *newifp;
>                     JSInterpreterHook hook;
> 
>+#define MAX_INLINE_CALL_COUNT 3000
>+                    /* Restrict recursion of lightweight functions. */
>+                    if (inlineCallCount == MAX_INLINE_CALL_COUNT) {
>+                        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                             JSMSG_OVER_RECURSED);
>+                        ok = JS_FALSE;
>+                        goto error;
>+                    }

One more thing: use js_ReportOverRecursed(cx) here, not JS_ReportErrorNumber(...).

>+
>                     /* Compute the total number of stack slots needed by fun. */
>                     nframeslots = JS_HOWMANY(sizeof(JSInlineFrame),
>                                              sizeof(jsval));
>                     nvars = fun->u.i.nvars;
>                     script = fun->u.i.script;
>                     atoms = script->atomMap.vector;
>                     nbytes = (nframeslots + nvars + script->depth) *
>                              sizeof(jsval);

Comment 15

[Brian Crowder]

On the google "search" page from bug 425555, this still takes a couple of seconds to recover (multiple different "too much recursion" errors), but I don't get the slow-script dialog anymore.

Comment 16

[Brian Crowder]

Attachment: with Igor's corrections

Comment 17

[Brian Crowder]

Comment on attachment 312751 [details] [diff] [review]
with Igor's corrections

Whoops, hit return too fast on the last one.

Comment 18

[Brian Crowder]

Comment on attachment 312751 [details] [diff] [review]
with Igor's corrections

Asking for a1.9, since we don't have blocking 1.9 yet.

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

How bad is it with 10,000 as the limit?  There's another browser with that limit, would be good to know if it's viable in the tests we have.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 312751 [details] [diff] [review]
with Igor's corrections

>Index: jsinterp.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
>retrieving revision 3.486
>diff -u -p -8 -r3.486 jsinterp.c
>--- jsinterp.c	29 Mar 2008 10:34:31 -0000	3.486
>+++ jsinterp.c	31 Mar 2008 17:08:25 -0000
>@@ -4612,16 +4612,23 @@ interrupt:
>                     uintN nframeslots, nvars, missing;
>                     JSArena *a;
>                     jsuword nbytes;
>                     void *newmark;
>                     jsval *newsp;
>                     JSInlineFrame *newifp;
>                     JSInterpreterHook hook;
> 
>+#define MAX_INLINE_CALL_COUNT 3000
>+                    /* Restrict recursion of lightweight functions. */

Nit: blank line before comment that takes one or more lines.

Non-nit: I hear Opera uses 10000, can we do the same without making the slow-script dialog too slow to appear?

/be

Comment 21

[Brian Crowder]

>+#define MAX_INLINE_CALL_COUNT 3000
>+                    /* Restrict recursion of lightweight functions. */

The #define here is almost like a part of the comment.  I added the space, it looked weird.  I put it after the comment, it looked weirder.  I'll add the space as per your nit, but it looks weird.

Testing 10K now.

Comment 22

[Brian Crowder]

10000 is fast enough (just barely, I'm pretty sure I was just shy of ten
seconds) to avoid the slow-script dialog on my machine (MBP), but since this
value -used- to be 1000, it seems like 3000 or 5000 would be a reasonable
compromise.  Does Opera's whole UI freeze while it is loading the google page?

Comment 23

[Damon Sicore (:damons)]

+'ing this.  Do we have a test here?  

Comment 24

[Brian Crowder]

function f() { f() }; f()

This should throw |InternalError: too much recursion|

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

No, it doesn't freeze during those ~10 seconds, which I think is quite relevant -- I think we should go to 3000, and that the failure to remove one difference in runtime limits between us and Opera is not a major problem.

This will also bear on bug 303821: Brendan, can we check this in at 3K today so that we get some pre-rc1 baking opportunity?

Comment 27

[Brendan Eich [:brendan]]

Sure, I'm not in the way. One nit: the macro definition still looks a bit odd in the attachment, and it is single-use. It used to be defined before js_Interpret. Not obviously better, but cvs annotate might benefit.

/be

Comment 28

[Brian Crowder]

Comment on attachment 312751 [details] [diff] [review]
with Igor's corrections

Yeah, I liked it better here, but I'm not attached.  Since this has blocking now I will go ahead and land it with MAX_INLINE_CALL_COUNT where it resided originally.

Comment 29

[Brian Crowder]

jsinterp.c: 3.488

Comment 30

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-424683-01.js,v  <--  regress-424683-01.js
initial revision: 1.1

Comment 31

[Bob Clary [:bc] (inactive)]

Note: this test is dependent upon architecture and available memory. On a x86_64 machine with 2G and a 64bit build, it will fail with InternalError: script stack space quota is exhausted however on a x86_64 with 4G and a 64bit build it will pass.

Comment 32

[Bob Clary [:bc] (inactive)]

fix file name in test:
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-424683-01.js,v  <--  regress-424683-01.js
new revision: 1.2; previous revision: 1.1

Comment 33

[Bob Clary [:bc] (inactive)]

v 1.9.0