./js -m js> Object.create(function(){}).apply(null, null); Assertion failure: simsp <= i.sp(), at js/src/jsfun.cpp:3061 The first bad revision is: changeset: d9aceaabef28 user: Luke Wagner date: Thu Oct 21 11:33:22 2010 -0700 summary: Bug 605192 - JM: make f.apply(x, obj) fast, part 4 (r=dvander)

The assertion is overly strict; a weaker one that should hold is: JS_ASSERT_IF(vp < simsp, vp < i.sp()); The reason that vp < i.sp() is that f.apply(x, null) turns into x.f() which ic::SplatApplyArgs achieves by popping the top 'null' before calling (with f as callee and x as this). This puts sp below what js_ReconstructStackDepth would expect. If js_fun_apply had the same in-place call optimization (instead of copying everything above sp), it would hit the same assert.

(In reply to comment #1) > The assertion is overly strict; a weaker one that should hold is: > JS_ASSERT_IF(vp < simsp, vp < i.sp()); > The reason that vp < i.sp() is that f.apply(x, null) turns into x.f() which > ic::SplatApplyArgs achieves by popping the top 'null' before calling (with f as > callee and x as this). This puts sp below what js_ReconstructStackDepth would > expect. If js_fun_apply had the same in-place call optimization (instead of > copying everything above sp), it would hit the same assert. Is it worth putting this explanation, or part of it, above the assertion?

Comment on attachment 493208 [details] [diff] [review] weaken assert What njn said -- comment please, and weaker assertion does not seem better if we don't need it yet. Let someone botch it, read the comment, then weaken. Or am I missing something in favor of weakening now? /be

Well, if we don't weaken now, it will continue to assert...

(in the testcase in comment 0)

Oh, ok -- weaken now, r=me. /be

Comment on attachment 493208 [details] [diff] [review] weaken assert For the record then?

I thought about it again and if, in general, the actual stack can be greater or less than the simulated stack, we just need to check for inclusion in the range [base, min(actual,simulated)) This patch does that and comments why.

Comment on attachment 497602 [details] [diff] [review] patch v.2 Great -- but where's the comment? /be

Oops, I post patches out of .hg/patches and I forgot to hg qref.

Comment on attachment 497628 [details] [diff] [review] patch v.2 (this time hg qref) > /* > * We try to the print the code that produced vp if vp is a value in the > * most recent interpreted stack frame. Note that additional values, not > * directly produced by the script, may have been pushed onto the frame's > * expression stack (e.g. by pushInvokeArgs) thereby incrementing sp past >- * the depth simulated by ReconstructPCStack. Since we must pass an offset >- * from the top of the simulated stack to js_ReportValueError3, it is >- * important to do bounds checking using the simulated, rather than actual, >- * stack depth. >+ * the depth simulated by ReconstructPCStack. Conversely, values may have New para before Conversely for better wrapping? >+ * been popped from the stack in preparation for a call (e.g., by >+ * SplatApplyArgs). Since we must pass an offset from the top of the >+ * simulated stack to js_ReportValueError3, it is important to do bounds Why switch to passive voice? "Since we must ..., we check bounds using ...". r=me with these picked. Thanks, /be

Sounds good http://hg.mozilla.org/tracemonkey/rev/6b7627584c94

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug614752.js.