356250 - "Assertion failure: !fp->fun || !(fp->fun->flags & JSFUN_HEAVYWEIGHT) || fp->callobj"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> (function() { eval("function() { }"); })()
Assertion failure: !fp->fun || !(fp->fun->flags & JSFUN_HEAVYWEIGHT) || fp->callobj, at jsinterp.c:480

Seems harmless in opt.

Usually bites the fuzzer within 2 iterations if I use it in debug.

The assertion was added in bug 352797.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix, needs some testing

This isn't just a bogus assertion.  Rather, for just about ever, eval has not had its frame's callobj (or argsobj) member forwarded from its callers.  But eval runs with dynamic scope, so it should.  This didn't matter because varobj was forwarded correctly, but for the assertion, and for greater consistency indicated by the terms in the assertion (no function or not heavyweight or callobj), we want a fix like this one.

/be

Comment 2

[Jesse Ruderman]

With the patch:

js> new Function("<x/>.('fafafa'.replace(/a/g, eval))")() 
Assertion failure: fp->scopeChain == parent, at jsfun.c:614

Comment 3

[Jesse Ruderman]

js> let(a) ((function() { let(b) (function(){ }); })());
Assertion failure: OBJ_GET_CLASS(cx, fp->scopeChain) != &js_BlockClass, at jsinterp.c:491

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix, v2

Thanks Jesse -- toldya it needed testing :-).

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix, v2a -- comment updates

Comment 6

[Brendan Eich [:brendan]]

Attachment: better fix

We do not want to force a Call object in js_GetScopeChain, because doing so would deoptimize function outer() {function inner(a, b){/*only a, b, arguments used*/} ...} so that outer would get a Call object, even though inner does not use non-local names.

Instead, we must either js_GetCallObject before js_GetScopeChain explicitly (as the jsdbgapi.c code does, via JS_GetFrameScopeChain) or implicitly (the parser arranging to set JSFUN_HEAVYWEIGHT, see jsparse.c).

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Created an attachment (id=242038) [edit]
> better fix
> 
> We do not want to force a Call object in js_GetScopeChain, because doing so
> would deoptimize function outer() {function inner(a, b){/*only a, b, arguments
> used*/} ...} so that outer would get a Call object, even though inner does not
> use non-local names.

Correction: the deoptimization if js_GetScopeChain were to force a Call object to reflect the frame immediately would hit named and anonymous function expressions:

function outer() {
  var inner = function (a, b) {/*only a, b, arguments*/}
  /* use inner here... */
}

Such lambdas *do* need to capture their lexical scope at runtime if enclosed in blocks, even if they don't use outer names, in case they contain lambdas that leak out somehow.  SpiderMonkey does not have the machinery to analyze data flow, so we have to pessimize somewhat currently.  But we should not regress the optimizations we do have, since real code does embed "pure" lambdas in outer functions, and we don't want to slow down such outer functions' activations.

/be

Comment 8

[Brendan Eich [:brendan]]

Fixed on trunk:

Checking in jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.69; previous revision: 3.68
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.299; previous revision: 3.298
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.258; previous revision: 3.257
done

/be

Comment 9

[Bob Clary [:bc] (inactive)]

Checking in regress-356250.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-356250.js,v  <--  regress-356250.js
initial revision: 1.1
done

Comment 10

[Bob Clary [:bc] (inactive)]

I don't see the assert for the testcase in this bug for 1.8.0, 1.8, 1.9 on 20061014 but did see it for the testcases in bug 356085, bug 355992 prior to 20061014 but no longer do.

verified fixed 1.9 20061014 windows/linux

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 242038 [details] [diff] [review]
better fix

approved for 1.8 branch, a=dveditz for drivers

Comment 12

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch:

Checking in jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.56.2.4; previous revision: 3.56.2.3
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.72; previous revision: 3.181.2.71
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.142.2.67; previous revision: 3.142.2.66
done

(I committed the fix for bug 352797 along with this followup fix.)

/be

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 20061122 1.8.1.1 windows/linux/mac*, 1.9 windows/linux