eval("let(y){(function(){let({}=y){(function(){let({}=y=[])(i)})()}})()}") crashes js opt shell on TM changeset e8ee411dca70 at CallPropertyOp without -j and asserts js debug shell at Assertion failure: FUN_FLAT_CLOSURE(callee_fun), s-s because this involves a scary address (0x40). === Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_PROTECTION_FAILURE at address: 0x00000000 0x0005890e in CallPropertyOp () (gdb) bt #0 0x0005890e in CallPropertyOp () #1 0x00084f65 in js_NativeSet () (gdb) x/i $eip 0x5890e <_ZL14CallPropertyOpP9JSContextP8JSObjectiPN2js5ValueE18JSCallPropertyKindi+158>: mov %eax,(%ecx,%esi,8) (gdb) x/b $eax 0x11023b8: 0x40 (gdb) x/b $ecx 0x0: Cannot access memory at address 0x0 (gdb) x/b $esi 0x0: Cannot access memory at address 0x0

Probably related to bug 558451. Regression window: http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=be9979b4c10b&tochange=f3e58c264932

Is this a (near-)null deref? or just happens to be in this testcase?

Ping?

Some kind of regression from the patch stack for bug 558451 (Gary, can you confirm?). It's thus very recent, so no branch worries. I will need more time to dig into this one. /be

(In reply to comment #4) > Some kind of regression from the patch stack for bug 558451 (Gary, can you > confirm?). It's thus very recent, so no branch worries. I will need more time > to dig into this one. > > /be Seems to be - the regression window is in comment #1.

1. http://deklomp.se/ 2. Assertion failure: FUN_FLAT_CLOSURE(callee_fun) This is associated with the socorro signature GetFlatUpvar. Although it only shows Linux (15) and Mac (1) in the last 30 days, I also see this on Windows. Search goodness: JS_Assert CallPropertyOp GetFlatUpvar js::CallJSPropertyOp js::Shape::get

based on deklomp.se page: still loads external scripts. requires meta tag.

Not a null deref for me. I see |array| as (js::Value *) 0xfff2000000000000 when I try the testcase in an opt-vg xpcshell on Linux64.

Brendan, any updates here yet?

eval("\ let(b)((\ function(){\ let(d=b)\ ((function(){\ b=b\ })())\ }\ )())\ ") asserts similarly and crashes opt shell at js::SetFlatUpvar without -m nor -j. Program received signal SIGSEGV, Segmentation fault. 0x080b43f8 in js::SetFlatUpvar(JSContext*, JSObject*, int, js::Value*) () (gdb) bt #0 0x080b43f8 in js::SetFlatUpvar(JSContext*, JSObject*, int, js::Value*) () #1 0x083396f0 in ?? () Backtrace stopped: previous frame inner to this frame (corrupt stack?) (gdb) x/i $eip => 0x80b43f8 <_ZN2js12SetFlatUpvarEP9JSContextP8JSObjectiPNS_5ValueE+56>: mov %esi,(%edx,%eax,8) (gdb) x/b $esi 0x0: Cannot access memory at address 0x0 (gdb) x/b $edx 0x0: Cannot access memory at address 0x0 (gdb) x/b $eax 0x0: Cannot access memory at address 0x0

bug 616762 looks like a dupe of this, though I like the reduced test case there better. At a glance, we could consider just removing MakeUpvarForEval. Chris is already removing most cases of the UPVAR ops in bug 614834, since ICs are faster.

bug 616762 looks unrelated, after all. The symptoms are very similar but the MakeUpvarForEval path isn't taken.

This still occurs in changeset df86d5999fef.

#6 and #9 on 4.0b9pre top crash list.

Bumping this to beta9, Gary believes that fixing this would take care of some other related crashes that appear in crahs-stats as well, and getting this in sooner rather than later would give us more time to react on those crashes if they turn out to not be fixed by this bug.

This is dopey, sorry for not looking into it sooner. The bug is an assertion and consequent requirements where there should be a guard with an early return true. The patch I'm about to attach minimizes change to avoid conflicting more than absolutely necessary with Waldo's specialized expansions of CallPropertyOp. /be

Removing scopes left compiler-created shape paths held by fun->u.i.names as precomputed scope replacements for all Call objects created for fun. The bug is that all UPVAR locals in a function have GetFlatUpvar and SetFlatUpvar as their getter and setter, to indicate their JSLocalKind. But unless the function is in fact a flat closure, these getter and setter functions should be no-ops. /be

There's more here than met my eye last night. The assumption was that we'd never have a Call object on the scope chain for a function that has upvars mapped as shapes if that function is not a flat closure. Such upvar-using lightweights make use of JSOP_*NAME and JSOP_GETUPVAR but do not themselves need Call objects. This assumption holds in all the tests if I revert the patch for bug 545573 to js_NewFlatClosure: -JSObject * +JS_REQUIRES_STACK JSObject * js_NewFlatClosure(JSContext *cx, JSFunction *fun) { - JSObject *closure = js_AllocFlatClosure(cx, fun, cx->fp->scopeChain); + /* + * Flat closures can be partial, they may need to search enclosing scope + * objects via JSOP_NAME, etc. + */ + JSObject *scopeChain = js_GetScopeChain(cx, cx->fp); + if (!scopeChain) + return NULL; + + JSObject *closure = js_AllocFlatClosure(cx, fun, scopeChain); I should have reverted this when I fixed bug 554572: changeset: 40000:77d5a7f48a4e user: Brendan Eich <brendan@mozilla.org> date: Wed Mar 24 13:26:28 2010 -0700 summary: Disable partial flat closures pending scope chain reconstruction on trace (554572, r=jorendorff; CLOSED TREE push ok'ed by sayrer). But since js_NewFlatClosure gets the full scope chain, it makes a Call object for any function containing a flat closure, even if that function has upvar shapes in its bindings. We don't do partial flat closures. That's a FIXME: bug 545759. Therefore when we make a flat closure, we do not need to reify the full scope chain including Call objects induced by block objects (which arise from catch clauses), and indeed we must not create Call objects for enclosing functions that are not themselves flat. This is all a bit too delicate, but I am testing a patch that should fix all the bugs and speed up flat closure computation. /be

> indeed we must not create Call objects for enclosing functions that are not > themselves flat. But this is of course impossible to prevent, at least due to the debugger -- see tests/ecma/extensions/trapflatclosure.js for one testcase. It's impossible to avoid also because we specialize JSOP_NAME to JSOP_GETUPVAR, ditto CALLNAME/CALLUPVAR, but the mutating variants do not have specialized UPVAR forms. If the function is heavyweight, these ops can't be used. If it escapes as a funarg, these two ops won't be emitted either. So it's easy to make tests that have JSOP_GETUPVAR but JSOP_SETNAME (the tests for this bug, Jesse's reduced one in particular) for the same upvar, and the JSOP_SETNAME works on a Call object for the function containing the upvar reference. But since scope removal such a function containing an upvar ref that's used by one or more JSOP_GETUPVARs will have a Call object property for the same upvar, in effect shadowing the upvar. The getter for this property won't be called (JSOP_GETUPVAR takes a faster, cough, maybe not so much without the display, path). But JSOP_SETNAME will call js::SetFlatUpvar. The symmetry break where JSOP_GET/CALLUPVAR are specialized but no mutating ops are, falling back on JSOP_SETNAME, etc., combined with independent causes of the function containing the upvar refs to need a Call object (e.g. it contains let or try/catch statements where the lexical scope is captured by a closure -- but not eval or with, those make heavyweights and eliminate the JSOP_GET/CALLUPVAR ops), means the compiler-precomputed upvar shapes -- the ones that were addUpvar'ed to bindings structs -- must shadow their actual upvars along the scope chain. So the setter, currently named js::SetFlatUpvar, must update the true home of the upvar, in the outer Call or Block (let in global scope) object. Patch is close. /be

http://hg.mozilla.org/try/rev/8735d09b21c4 looks good to my eyes -- unstarred win dbg reftest is Number.toLocaleString known badness, right? /be

http://tbpl.mozilla.org/?tree=MozillaTry&rev=8735d09b21c4 looks more than good, it looks jaw-droppingly green. Pushed again with "try: -a" in the commit message and rebased with the .toLocaleString fix in http://tbpl.mozilla.org/?tree=MozillaTry&rev=f9d2a25708b8 since without -a or explicitly asking for it you don't get Talos, and other than a couple of busted slaves it's happy too.

http://hg.mozilla.org/try/rev/dcc047f8e36e per comment 28. /be

What were the results from the tryserver run? Is it ready to check in?

Fine, pretty much the same as when I had already done it, since I was saying "I did" rather than "you should." But it's not ready to check in since it hasn't yet been reviewed.

Phil, ever seen anything like this? s: talos-r3-snow-021 TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-290575.js | application timed out after 330 seconds with no output from my last push to try. Will try to reproduce. /be

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Fixing fields my automated script accidentally blanked. Apologies for the bugspam

No, I haven't ever seen regress-290575.js time out (including during the other two times you and I pushed it to try), though bug 613551 did manage to break it today, in a way I didn't understand.

I don't know that this characterizes as an unshippable trunk BUT as I understand it our goal for each beta is NOT to regress on crashes. This one is responsible for a bunch of signatures. Any chance we can get it in for beta9?

I'm waiting for code review. Jason has been sick but I saw mail from him today. This absolutely should block beta9, time-based release is not an absolute. /be

In jsfun.cpp, GetCallUpvar: >+ /* >+ * Our callee must be a flat closure. Any other kind of upvar-referencing >+ * function will use JSOP_GETUPVAR or JSOP_CALLUPVAR, or else won't ever >+ * have upvar shapes in its compiler-created shape lineage which is shared >+ * by all Call objects created for activations of the function. >+ */ > *vp = obj->getCallObjCallee().getFlatClosureUpvar(i); > return true; > } To expand the proof a bit: 1. We only use the GetCallUpvar getter on Call objects for functions that qualify for GETUPVAR/CALLUPVAR access to the referenced arguments/variables. (In particular, obj->getCallObjCallee() is non-escaping w.r.t. the scope of the arg/var.) 2. These properties are only ever got directly, via bytecode instructions. (This assumes the debugger doesn't try to grab arbitrary properties off the scope object. I don't remember it being that well-behaved; is it?) 3. Furthermore, if we add a binding for an upvar, we get the upvar exclusively via JSOP_GETUPVAR and CALLUPVAR, except in flat closures. We never emit unspecialized NAMEish opcodes that might call the getter. (What about INCNAME? What about JS_EvaluateInStackFrame code that doesn't qualify for the upvar optimization?) If 1, 2, and 3 were true, we could conclude that the callee here is a flat closure. At least INCNAME seems like a hole. In js_NewFlatClosure: >+ * FIXME: bug 545759 proposes to enable partial flat closures. Fixing this >+ * bug requires a GetScopeChainFast call here, along with JS_REQUIRES_STACK >+ * annotations on this function's prototype and definition. > */ >- JSObject *scopeChain = GetScopeChainFast(cx, cx->fp(), op, oplen); >- if (!scopeChain) >- return NULL; >+ JSObject *scopeChain = &cx->fp()->scopeChain(); The call to cx->fp() would run afoul of the static analysis. In this case, you don't care exactly which stack frame you get, as long as the right global object is at the end of the scope chain, right? In that case, add: VOUCH_DOES_NOT_REQUIRE_STACK(); before that line. In class Bindings: >+ * before before any VARIABLE or CONSTANT bindings, which themselves must "before" is doubled.

That lightened the load! Trying to test, need orange to go away and then to rebase and push to try. But please feel free to review, this simplifies away the whole problem, and removes a lot of code including my nemesis from the early tofte wars, MakeUpvarForEval. /be

(function() { let(x) function() { let(a = x) function() { x = x } () } () } ()) asserts js debug shell at Assertion failure: fun->isFlatClosure(), and crashes opt shell at js::SetFlatUpvar. I haven't tested this against the latest patch, though.

Comment 42 occurred on TM changeset ca11457ed5fe without -m nor -j. This is another testcase showing similar issues as comment 42: let(b)((function() { let([] = <x/>, x = b, d = b = "")(function() {}) })())

Gary, there are tons of testcase variations on a theme. No need to put more in this bug, especially when the patch fixes them all. /be

Does that mean we are done? Can we wrap up this one today before the beta9 freeze?

Morning push to try at 932a856611fb is looking good (I used try: -a), save for the patch exposing two latent bugs in tests: js1_8_1/extensions/regress-452498-196.js js1_8_1/regress/regress-452498-119.js Both relied on eval'ed const redeclaring a local var, naughty. Fixing those to use var in the eval. /be

"Relied on"? They were tests for assertions and crashes, so replacing "const" with "var" could change what they test. Please add a new test covering the intentional behavior that affects the old version of those tests.

I get a clean js/src/tests run in my debug shell with this patch, clean in that only these tests fail: js1_5/extensions/regress-336409-1.js js1_6/extensions/regress-456826.js I think these are known failures. Wish I could be sure without unpatching and rerunning (and hoping they aren't intermittent). /be

(In reply to comment #47) > "Relied on"? They were tests for assertions and crashes, so replacing "const" > with "var" could change what they test. The tests say what they were checking, but they cannot pass now as written. > Please add a new test covering the intentional behavior that affects the old > version of those tests. The old versions throw SyntaxErrors. Wrapping the evals in try/catch could work but would change the generated bytecode more fundamentally (catch makes a block scope). Changing to var and avoiding try/catch preserves what these were about. /be

(In reply to comment #49) > (In reply to comment #47) > > "Relied on"? They were tests for assertions and crashes, so replacing "const" > > with "var" could change what they test. > > The tests say what they were checking, IOW, I read the tests and made the right changes. But thanks for the doubt! :-/ /be

Comment on attachment 502134 [details] [diff] [review] proposed fix, rebased and with tests fixed In comment 40 I wrote a bit about JS_REQUIRES_STACK. That applies to this patch too. Also I'd like another follow-up bug to look at the `if (!fn->isFunArg())` block in Parser::setFunctionKinds. I bet those two blocks of code (the if block and the else block) can be combined into something shorter and saner. > if (!lexdep->isFreeVar()) { > JS_ASSERT(lexdep->frameLevel() <= funbox->level); > ++nupvars; >- if (lexdep->isAssigned()) >- break; > } I think this could just say `break;`. The purpose of the loop is to find out if nupvars is zero or not; at this point we know it's not. r=me.

(In reply to comment #48) > I get a clean js/src/tests run in my debug shell with this patch, clean in that > only these tests fail: > > js1_5/extensions/regress-336409-1.js > js1_6/extensions/regress-456826.js > > I think these are known failures. Wish I could be sure without unpatching and > rerunning (and hoping they aren't intermittent). 336409 is known to suck, as reported in 618337 456826 is believed to be invalid, per bug 594667. We should lose both those tests, IMO, but I'll take it to the bugs in question.

(In reply to comment #52) > Comment on attachment 502134 [details] [diff] [review] > proposed fix, rebased and with tests fixed > > In comment 40 I wrote a bit about JS_REQUIRES_STACK. That applies to this patch > too. We lost tinderbox coverage of this static analysis, which means it has rotted. Can we get that back? Or 'make check' on Linux with uprev gcc does it for you? /be

(In reply to comment #52) > I'd like another follow-up bug to look at the `if (!fn->isFunArg())` block > in Parser::setFunctionKinds. I bet those two blocks of code (the if block and > the else block) can be combined into something shorter and saner. As mentioned on IRC, that condition is a stricter one than the condition we need now: that the function isn't induced by a hoisted declaration form. We cannot flatten non-lambdas. > > if (!lexdep->isFreeVar()) { > > JS_ASSERT(lexdep->frameLevel() <= funbox->level); > > ++nupvars; > >- if (lexdep->isAssigned()) > >- break; > > } > > I think this could just say `break;`. The purpose of the loop is to find out if > nupvars is zero or not; at this point we know it's not. Right, fixing. > r=me. Thanks! /be

http://hg.mozilla.org/tracemonkey/rev/a213cb8ca396 /be

JSBugMon: This bug has been automatically verified fixed.

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-592202-3.js.