Closed Bug 424683 Opened 14 years ago Closed 14 years ago

script stack space quota is exhausted

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: jmjjeffery, Assigned: crowderbt)

References

()

Details

(Keywords: hang, regression)

Attachments

(1 file, 2 obsolete files)

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 ?
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?
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 
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
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.)
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.
Flags: blocking1.9?
Depends on: 392973
This needs more testing.
Assignee: general → crowder
Status: NEW → ASSIGNED
This bug is probably also fix for hang problem described in bug 303821 comment 57; and also for the crash its self.
Blocks: 303821
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. :)
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.
Hm, this patch is broken.  I could have sworn I'd uploaded a newer version.  Will repair when I've finished testing.
Attached patch better (obsolete) — Splinter Review
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.
Attachment #312190 - Attachment is obsolete: true
Attachment #312663 - Flags: review?(shaver)
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 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);
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 on attachment 312751 [details] [diff] [review]
with Igor's corrections

Whoops, hit return too fast on the last one.
Attachment #312751 - Attachment description: with → with Igor's corrections
Attachment #312751 - Flags: review?(igor)
Attachment #312751 - Attachment is patch: true
Attachment #312751 - Attachment mime type: application/octet-stream → text/plain
Attachment #312663 - Attachment is obsolete: true
Attachment #312663 - Flags: review?(shaver)
Attachment #312751 - Flags: review?(igor) → review+
Comment on attachment 312751 [details] [diff] [review]
with Igor's corrections

Asking for a1.9, since we don't have blocking 1.9 yet.
Attachment #312751 - Flags: approval1.9?
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 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
>+#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.
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?
+'ing this.  Do we have a test here?  
Flags: blocking1.9? → blocking1.9+
function f() { f() }; f()

This should throw |InternalError: too much recursion|
Duplicate of this bug: 426592
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?
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 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.
Attachment #312751 - Flags: approval1.9?
jsinterp.c: 3.488
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-424683-01.js,v  <--  regress-424683-01.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-
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.
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
Blocks: 428386
OS: Windows Vista → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9
v 1.9.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.