472450 - TM: Crash [@ JS_GetGlobalForObject] or [@ js_PutBlockObject] or [@ js_Interpret]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

After bug 470388 was fixed, the following testcases remain unfixed and crash at the following locations in opt: ( from https://bugzilla.mozilla.org/show_bug.cgi?id=470388#c6 )

JS_GetGlobalForObject at 0x000000000000000c:

for each (let x in [function(){}, {}, {}, function(){}, function(){},
function(){}]) { ([<y/> for (y in x)]); }

======

js_PutBlockObject at null:

for each (let x in [function(){}, {}, {}, function(){}, function(){}]) { ([<y/>
for (y in x)]); }

======

Both testcases assert dbg at:
Assertion failure: StackBase(fp) + blockDepth == regs.sp, at ../jsinterp.cpp:6732

in TM.

Comment 1

[Jesse Ruderman]

Bug 472941 has another testcase that triggers this assertion.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Should you need an additional test case -- http://apps.goonfleet.com/evetools/route.html and uncheck/recheck the "Use normal jump bridges" checkbox.  Crashes on checking it, e.g.: http://crash-stats.mozilla.com/report/index/83ba79ed-8663-4074-8f85-3c1d32090111

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

These JS_GetGlobalForObject and js_PutBlockObject crashes in opt are occurring significantly often when running jsfunfuzz.

Comment 4

[Jim Blandy :jimb]

Okay.  I'm actively working on this.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

For automated testing, here's a testcase that's slightly different but has the same outcomes in shell:

({__proto__: #1=[#1#]})
for (var y = 0; y < 3; ++y) { for each (let z in ['', function(){}]) { let x = 1, c = []; } }

crashes opt TM at JS_GetGlobalForObject at 0x000000000000000c and also asserts at Assertion failure: StackBase(fp) + blockDepth == regs.sp, at ../jsinterp.cpp:6724

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

({__proto__: #1=[#1#]})
function f(){ eval("for (var y = 0; y < 1; ++y) { for each (let z in [null, function(){}, null, '', null, '', null]) { let x = 1, c = []; } }"); }
f();

crashes opt TM at js_Interpret at null and asserts identically: Assertion failure: StackBase(fp) + blockDepth == regs.sp, at ../jsinterp.cpp:6724

Comment 7

[Robert Sayre]

jim, what's up here?

Comment 8

[Jim Blandy :jimb]

It seems like JITted code is not properly managing the scope chain.  The code includes two nested lexical blocks.  At some point after having left jitted code, JSOP_LEAVEBLOCK checks that the stack has the expected height, which it does (and the proper contents) --- but the scope chain contains only the outer block and the global block.  The inner block that JSOP_LEAVEBLOCK expects to pop is already gone.

Comment 9

[Jim Blandy :jimb]

So, here's the disassembly:

// js> disfile('/home/jimb/mc/b/glob/t1.js')
// main:
// 00000:  enterblock depth 0 {x: 0}  
// 00003:  anonfunobj (function () {})
// 00006:  newinit 1 (JSProto_Object) 
// 00008:  endinit                    
// 00009:  newinit 1 (JSProto_Object) 
// 00011:  endinit                    
// 00012:  anonfunobj (function () {})
// 00015:  anonfunobj (function () {})
// 00018:  anonfunobj (function () {})
// 00021:  newarray 6                 
// 00025:  iter 3 (ENUMERATE|FOREACH) 
// 00027:  goto 67 (40)               
// 00030:  forlocal 0                 
// 00033:  newinit 3 (JSProto_Array)  
// 00035:  enterblock depth 4 {y: 0}  
// 00038:  getlocal 0                 
// 00041:  iter 1 (ENUMERATE)         
// 00043:  goto 57 (14)
// 00046:  forlocal 4                 
// 00049:  startxml                   
// 00050:  string "<y/>"              
// 00053:  toxml                      
// 00054:  arraypush 3                
// 00057:  nextiter                   
// 00058:  ifne 46 (-12)              
// 00061:  enditer                    
// 00062:  leaveblock 1               
// 00065:  endinit                    
// 00066:  pop                        
// 00067:  nextiter                   
// 00068:  ifne 30 (-38)              
// 00071:  enditer                    
// 00072:  leaveblock 1               
// 00075:  stop                       

The trace starts at 30, and we leave trace from a call to ObjectToIterator_tn, presumably from the JSOP_ITER at 41.  However, although that iter instruction is in the scope of the inner lexical block (35--62), the VMSideExit record passed to LeaveTree has a null block, so LeaveTree doesn't reconstruct fp's block chain.

So, either we're not constructing the VMSideExit record properly, or we're being passed the wrong one.

Comment 10

[Andreas Gal :gal]

I have an idea. Let me try something.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Once js_GetScopeChain has been called, we can no longer properly restore the block chain.

This patch is slightly overzealous in that something like:
function f() { let (x) { (function(){}); } for (let i = 0; i < 5; ++i) print(i) } will not be traced anymore. If we did enough work to figure out that we could unset JSFRAME_POP_BLOCKS when the first let statment leaves, then we could avoid this. I doubt this is a huge problem, though.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Sorry, that example should be:

var global;
function f() { let (x) { global = function(){}; } for (var i = 0; i < 5; ++i) { let k = i; print(k + '') } }

Comment 13

[Andreas Gal :gal]

Comment on attachment 361844 [details] [diff] [review]
Fix

No slowdown on SS.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/592836729d33

Comment 15

[Jason Orendorff [:jorendorff]]

Is this fix enough?  It seems possible with this patch to record a trace, then call js_GetScopeChain from the interpreter, then try to execute that trace (even though JSFRAME_POP_BLOCKS is set now).

Honestly I'm a little lost as to why we can't recover, but that's a separate issue.

Comment 16

[Jason Orendorff [:jorendorff]]

(summarizing IRC conversations)

I'm not opposed to deciding we don't want to make this work properly right now.  I think there are several bugs in the current code, and upvar might mess up some of the assumptions anyway.

Bug #1: JSFRAME_POP_BLOCKS might be unset at record time (when exit->block is captured) but set at trace-entry time.  Fix: in snapshot, if JSFRAME_POP_BLOCKS is unset, go digging around in the script and find the appropriate Block object.

Bug #2: snapshot initializes exit->block using the current top frame.  But for correctness, exit->block must refer to the entry frame.  (Any frames on top of it can't have JSFRAME_POP_BLOCKS set, as that can't happen on trace--the frames simply don't exist to have js_GetScopeChain called on them.)

Bug #3: Likewise, LeaveTree needs to set entryFrame->blockChain, not cx->fp->blockChain after new frames are synthesized.

If there's a followup patch here, please mark all that code /*FIXME*/ or just delete it.  It is (we hope) dead code at the moment.

Comment 17

[Jason Orendorff [:jorendorff]]

OK, piling on, but Bug #4:  I think neither lr nor innermost is the right VMSideExit to use to fix up entryFrame->blockChain in LeaveTree.  Rather, we need to find the innermost VMSideExit that has the same entry frame as lr.  So in code like this:

function f() {
  while (loop1) {
    let ...
    while (loop2) {
      let ...;
      g();
    }
  }
}

function g() {
  while (loop3) {
    ...
  }
}

If we record all three loops, then start executing loop1 and bail out inside loop3, we have to use the exit->block from when we bailed out of loop2.

Comment 18

[Jim Blandy :jimb]

Even with the patch pushed in comment #14 applied, the following will cause LeaveTree to set fp->blockChain to the compiler-allocated block object for the innermost block, even though fp->flags & JSFRAME_POP_BLOCKS is true and fp->scopeChain has mutable blocks on it.  This leads eventually to:

Assertion failure: !js_IsActiveWithOrBlock(cx, fp->scopeChain, 0), at ../jsinterp.cpp:7150

for (let i = 1; i < 10; i++) {
  let t=0;
  for (let j = 1; j < 20; j++) {
    t += j;
  }
  t = (function(){});
}

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #18)
> Even with the patch pushed in comment #14 applied, the following will cause
> LeaveTree to set fp->blockChain to the compiler-allocated block object for the
> innermost block, even though fp->flags & JSFRAME_POP_BLOCKS is true and
> fp->scopeChain has mutable blocks on it.  This leads eventually to:
> 
> Assertion failure: !js_IsActiveWithOrBlock(cx, fp->scopeChain, 0), at
> ../jsinterp.cpp:7150
> 
> for (let i = 1; i < 10; i++) {
>   let t=0;
>   for (let j = 1; j < 20; j++) {
>     t += j;
>   }
>   t = (function(){});
> }

Is this testcase and assertion related to bug 472528, which Brendan indicates will be fixed by the heavyweight upvar patch?

Comment 20

[Jim Blandy :jimb]

Yes, it looks the same as that test case.

Comment 21

[IU]

This crash is now being seen with today's Trunk build, when typing in the address bar (if JIT chrome is enabled).  Crash signature is: [@ js_PutBlockObject ]: http://crash-stats.mozilla.com/report/index/eff0ea8e-5bdd-46fa-b465-b6de42090217

When will this be merged?

Comment 22

[IU]

By the way, the crashes are happening on Windows.

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/592836729d33

Comment 26

[[e-mail] (:bhearsum)]

Does this still have to land on 1.9.1? I just hit this on mozilla-1.9.1/20090218

Comment 29

[Tony Chung [:tchung]]

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090218 Minefield/3.2a1pre.  Awaiting fix to land on 1.9.1

Comment 30

[[e-mail] (:bhearsum)]

Can we get this in post-haste so we can respin the nightlies?

Comment 31

[Laurence "GreenReaper" Parry]

For the benefit of those having problems due to this bug: setting jit.chrome to false temporarily in about:config will workaround this problem.

One way to get to about:config without typing is to make a desktop shortcut to any website (just type one in), then change its URL in right-click/properties to about:config. You can then drag this onto your browser window. Another is to make and load a new bookmark for about:config with the bookmark organizer in the Tools menu.

Comment 32

[Segaja]

typing about:config worked for me. only normal other urls caused the crash.

Comment 33

[Patrick Tulskie]

I am having this on OS X with Shiretoko nightly Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090218 Shiretoko/3.1b3pre Ubiquity/0.1.4

Turning off JIT for Chrome does indeed fix it for the time being.  Will this fix be in the nightly for 1.9.1 released tomorrow?  Interestingly enough, this bug didn't show up for me until the build I am running right now.

Comment 37

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/95e7c2d17a5a

Comment 38

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Prior to the checkin on 1.9.1, this also caused js_PutBlockObject crashes in TB:

http://crash-stats.mozilla.com/report/index/c9652198-2949-4dd9-93f1-d527e2090228
http://crash-stats.mozilla.com/report/index/f33ac5de-9bd0-4882-a8f8-7795a2090222

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on 1.9.1 based on current crash reports. The latest reported crash happened with build 20090226034605.

Comment 41

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/1ab1141aa294
/cvsroot/mozilla/js/tests/js1_8/regress/regress-472450-01.js,v  <--  regress-472450-01.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/regress/regress-472450-02.js,v  <--  regress-472450-02.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-472450-03.js,v  <--  regress-472450-03.js
initial revision: 1.1
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-472450-04.js,v  <--  regress-472450-04.js
initial revision: 1.1