605011 - "Assertion failure: JS_CHECK_STACK_SIZE(cx->stackLimit, &stackDummy),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function g(x, n) {
  for (var i = 0; i < n; ++i) {
    x = {
      a: x
    };
  }
  return x;
}
d = g(0, 672);
(function() {
  gczeal(2)(uneval(this))
})()

asserts js debug shell on TM changeset 47a8311cf0bb without -m or -j at Assertion failure: JS_CHECK_STACK_SIZE(cx->stackLimit, &stackDummy),

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Also, locking s-s due to presence of gczeal in testcase.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54771:fd4510c77054
user:        Gregor Wagner
date:        Tue Oct 05 10:09:50 2010 -0700
summary:     Bug 600310 - TM: don't perform GC outside of stack quota r=igor

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

This is the same as seen in bug 600139, an edge case where the assert is guaranteed to hit. Throwing an over-recursed error can cause a GC, and while you're overrecursed, you're probably beyond the stack limit.

I don't know whether it's a security issue or not though.

Comment 3

[Igor Bukanov]

I think we should replace the assert from the bug 600139, with one that alows for few more kilobytes of the stack space. That is, the functions that sets the stack limit should make it slightly smaller so the GC will have an extra room after the error.

Gregor, do you have time to patch this?

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

This patch adds 4k to the stackLimit.
Another possibility would be to check if we are already in an over-recursion error.

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

It seems cx->stackLimit is not always set (jsapi-tests).

Comment 6

[Igor Bukanov]

Comment on attachment 484423 [details] [diff] [review]
patch

>diff -r 5bca7c8bd4c8 js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp	Tue Oct 19 11:39:55 2010 -0700
>+++ b/js/src/jsgc.cpp	Tue Oct 19 12:49:16 2010 -0700
>@@ -2558,17 +2558,22 @@ js_GC(JSContext *cx, JSGCInvocationKind 
>      */
>     if (rt->state != JSRTS_UP && gckind != GC_LAST_CONTEXT)
>         return;
> 
>     RecordNativeStackTopForGC(cx);
> 
> #ifdef DEBUG
>     int stackDummy;
>-    JS_ASSERT(JS_CHECK_STACK_SIZE(cx->stackLimit, &stackDummy));
>+/* +-4k because it is possible to perform a GC during an overrecursion report. */
>+# if JS_STACK_GROWTH_DIRECTION > 0
>+    JS_ASSERT_IF(cx->stackLimit, JS_CHECK_STACK_SIZE(cx->stackLimit + 4096, &stackDummy));

For upward growing stacks cx->stackLimit is jsuword(-1) by default, the max value. r+ with that fixed.

Comment 7

[Gregor Wagner [:gwagner]]

http://hg.mozilla.org/tracemonkey/rev/0bb000635fbd

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> http://hg.mozilla.org/tracemonkey/rev/0bb000635fbd

I was not clear in the comment 5. The suggestion was to change the assert into:

JS_ASSERT_IF(cx->stackLimit != jsuword(-1), JS_CHECK_STACK_SIZE(cx->stackLimit + 4096, &stackDummy));

Comment 9

[Gregor Wagner [:gwagner]]

Yeah that makes more sense. Thx!
http://hg.mozilla.org/tracemonkey/rev/0ae037c29ab0

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0ae037c29ab0

Comment 11

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929