./js -j c.js Assertion failure: pendingGlobalSlotToSet == -1, at /Users/jruderman/tracemonkey/js/src/jstracer.cpp:3689 The first bad revision is: changeset: 38f985b0522c user: Luke Wagner date: Thu Nov 18 10:49:45 2010 -0800 summary: Bug 561954 - Abort recording earlier to avoid expensive later bails (r=jorendorff)

This test triggers the same assert: --- var a, b; function g(x) { var y = x++; return [x, y]; } function f() { for(var i=0; i<20; i++) { [a,b] = g("10"); } } f(); ---

Ah, it seems there can be more than one global write per recorded op; should've seen this coming. *sigh* this tracer design is killing me. I think the solution is to turn pendingGlobalSlotToSet into a Vector<int>. Until then, this assert can be safely commented out; its not a correctness bug, it will just cause some unnecessarily-pessimistic trace aborts.

While the potential for real exploits is low, I can see the assert hitting in the wild and wasting debug-build users' time, so I'm going mark this blocking.

Blah. The assertion happens under TR::closeLoop, in SlotMap::adjustType, right? If so, we do not expect the interpreter to write to these global slots. It won't even get a chance; we're done recording. It seems more direct to skip the pendingGlobalSlotToSet code (and this assert) if we're in closeLoop.

Oh, hah, I hadn't even checked the backtrace; just that the testcase was fixed. Other than through enumeration, I can't find any hard reason why there can't be multiple writes to a global in a single op, so I'd almost rather just put in the general form, unless you think that there is no remotely likely chance, now or ever of this occurring outside closeLoop.

Comment on attachment 500915 [details] [diff] [review] fix Hmm. Well... all things being equal, I'd rather remove the assertion or change it to an `if (assumption violated) RETURN_STOP();`, than have a bit of machinery we think can't be used at present, and have no particular reason to think will ever be useful. However, we are in a hurry. I'll leave this to your judgment. The patch looks correct. r=me.

(In reply to comment #8) > Hmm. Well... all things being equal, I'd rather remove the assertion or change > it to an `if (assumption violated) RETURN_STOP();`, than have a bit of > machinery we think can't be used at present, and have no particular reason to > think will ever be useful. I would agree and initially considered this but: (1) there is no easy way to RETURN_STOP() from Tracker::setImpl; certainly we'd not want to force all callers of set() to check and if we only made the global-setting callers check, we'd be back to my old grosser patch that had to find every place we set a global in the tracer, (2) we'd still need to insert some special case "if we are closing the loop" predicate which would add another ounce of hack. So, despite its lack of foreseeable use outside closeLoop, I think this patch is the least hacky route.

Automatically extracted testcase for this bug was committed: https://hg.mozilla.org/mozilla-central/rev/efaf8960a929