391033 - "Assertion failure: (size_t)nslots >= (size_t)(fp->down->sp - vp)" with gczeal and trying to call a non-function

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> gczeal(2); ({})();
Assertion failure: (size_t)nslots >= (size_t)(fp->down->sp - vp), at jsgc.c:2000

Comment 1

[Igor Bukanov]

Here is a test case that does not use gczeal but rather invokes gc at the right place:

~/m/trunk/mozilla> js -e '({toSource: gc})()'
Assertion failure: (size_t)nslots >= (size_t)(fp->down->sp - vp), at jsgc.c:2000
Trace/breakpoint trap

Comment 2

[Igor Bukanov]

The bug is regression from my fix for bug 390078. The reason for it is that ReportIsNotFunction from jsobj.c removes the current frame for a native call from JS stack and put it to the dormant list. Thus the previous frame becomes the top of the stack again. When later js_Invoke is eventually called to invoke toSource on the function object, the available stack space on that frame is used for arguments and extras for the call producing a stack layout like:

previousFrame.spbase ... dormantFrame.argv[-2] dormantFrame.argv[-1] toSourceFrame[-2] .. toSourceFrame[extra0] .. previousFrame.sp

Thus fp->argv can be a proper subsegment of fp->down->spbase..fp->down->sp and the assert is bogus.

Comment 3

[Igor Bukanov]

Attachment: fix v1

Besides the fix itself the patch also changes js_TraceContext not to link the active frames to the dormant chain even temporarily. For me it simplified life when debugging.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 275472 [details] [diff] [review]
fix v1

>@@ -1992,18 +1993,29 @@ js_TraceStackFrame(JSTracer *trc, JSStac
>             /*
>              * Avoid unnecessary tracing in the common case when args overlaps
>              * with the stack segment of the previous frame. That segment is
>              * traced via the above spbase code and, when sp > spbase + depth,
>              * during tracing of the stack headers in js_TraceContext.
>              */
>             if (JS_UPTRDIFF(vp, fp->down->spbase) <
>                 JS_UPTRDIFF(fp->down->sp, fp->down->spbase)) {
>-                JS_ASSERT((size_t)nslots >= (size_t)(fp->down->sp - vp));
>-                nslots -= (uintN)(fp->down->sp - vp);
>+                nslots -= fp->down->sp - vp;
>+                if (nslots < 0) {
>+                    /*
>+                     * This is possible when, for example, ReportIsNotFunction
>+                     * from jsobj.c moves cx->fp to a dormant list before

"moves cx->fp to cx's dormant list" or "moves cx->fp to cx->dormantFrameChain" even.

>+                     * calling js_ReportIsNotFunction. When the latter calls
>+                     * js_InternalInvoke to invoke toSource, 2 frames points

"two frames point"

>+                     * to fp->down and fp->down->spbase..sp may contain the

comma after "to fp->down"

>+                     * argument arrays for both. It leads to a negative nslots
>+                     * here for the dormant frame.
>+                     */
>+                    nslots = 0;

Is this sufficient, or could we end up overscanning a little bit? It seems more attractive to cope with the odd ReportIsNotFunction case by detecting it more directly. Or else get rid of that odd case ;-).

>+    /* The top frame can not be dormant. */

"cannot" or (better) "must not".

/be

Comment 5

[Igor Bukanov]

Attachment: fix v2

The new fix removes ReportIsNotFunction and calls js_ReportIsNotFunction directly from js_Call/js_Construct. This should not affect error reporting as js_ReportIsNotFunction searches for interpreter frames on its own and calling js_Call/js_Construct is always down through a native frame. Thus removal of the top stack frame is effectively no-op from error handling point of view.

Comment 6

[Brendan Eich [:brendan]]

What was the original reason for ReportIsNotFunction? There must have been a case that seemed to want to hide the top frame. Ah, here's the commit info:

revision 3.99
date: 2001/09/05 21:25:09;  author: jband%netscape.com;  state: Exp;  lines: +30 -13
fix bug 97444. It is not good to patch a different fun into the frame. Let's safely shunt aside the callee frame instead. r=rogerl sr=brendan

Does your patch regress bug 97444 ?

/be

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> What was the original reason for ReportIsNotFunction? There must have been a
> case that seemed to want to hide the top frame. Ah, here's the commit info:
> 
> revision 3.99
> date: 2001/09/05 21:25:09;  author: jband%netscape.com;  state: Exp;  lines:
> +30 -13
> fix bug 97444. It is not good to patch a different fun into the frame. Let's
> safely shunt aside the callee frame instead. r=rogerl sr=brendan
> 

I should remember the next time to check and comment it.

> Does your patch regress bug 97444 ?

No: with the invention of jSStackFrame.callee the problem described in bug 97444 comment 11 is no longer exists as the js_Call does not manipulates the callee field.

Comment 8

[Brendan Eich [:brendan]]

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

Great to see that code go away.

/be

Comment 9

[Igor Bukanov]

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

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

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-391033-01.js

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-391033-02.js

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.

Comment 13

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.