Closed Bug 356250 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: jruderman, Assigned: brendan)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(1 file, 3 obsolete files)

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.
Attached patch fix, needs some testing (obsolete) — Splinter Review
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
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #242003 - Flags: review?(mrbkap)
With the patch:

js> new Function("<x/>.('fafafa'.replace(/a/g, eval))")() 
Assertion failure: fp->scopeChain == parent, at jsfun.c:614
js> let(a) ((function() { let(b) (function(){ }); })());
Assertion failure: OBJ_GET_CLASS(cx, fp->scopeChain) != &js_BlockClass, at jsinterp.c:491
Attached patch fix, v2 (obsolete) — Splinter Review
Thanks Jesse -- toldya it needed testing :-).

/be
Attachment #242003 - Attachment is obsolete: true
Attachment #242035 - Flags: review?(mrbkap)
Attachment #242003 - Flags: review?(mrbkap)
Attached patch fix, v2a -- comment updates (obsolete) — Splinter Review
Attachment #242035 - Attachment is obsolete: true
Attachment #242036 - Flags: review?(mrbkap)
Attachment #242035 - Flags: review?(mrbkap)
Attached patch better fixSplinter Review
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
Attachment #242036 - Attachment is obsolete: true
Attachment #242038 - Flags: review?(mrbkap)
Attachment #242036 - Flags: review?(mrbkap)
No longer blocks: 352797
Depends on: 352797
OS: Mac OS X 10.4 → All
Priority: -- → P1
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.9alpha
(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
Attachment #242038 - Flags: review?(mrbkap) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #242038 - Flags: approval1.8.1.1?
Flags: blocking1.8.1.1?
Checking in regress-356250.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-356250.js,v  <--  regress-356250.js
initial revision: 1.1
done
Flags: in-testsuite+
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
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment on attachment 242038 [details] [diff] [review]
better fix

approved for 1.8 branch, a=dveditz for drivers
Attachment #242038 - Flags: approval1.8.1.1? → approval1.8.1.1+
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
Keywords: fixed1.8.1
Keywords: fixed1.8.1fixed1.8.1.1
verified fixed 20061122 1.8.1.1 windows/linux/mac*, 1.9 windows/linux
No longer blocks: 349611
Blocks: 349611
You need to log in before you can comment on or make changes to this bug.