672804 - "Assertion failure: parent,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

function f() {
    let(a = [[] for (x in e)], {
        b: []
    } = x) {}
}
dis(f)
trap(f, 4, '')
f()

asserts js debug shell on MI changeset 47d8748daa90 with -m, -a and -d at Assertion failure: parent,

This assert mutated from an earlier assertion: Assertion failure: index < arr->length,

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   72780:69dfa4008531
user:        Jason Orendorff
date:        Thu Jul 14 08:54:17 2011 -0500
summary:     Bug 646968 - Fix name lookups in let scopes. r=brendan.

===

js> dis(f)
flags: NULL_CLOSURE
loc     op
-----   --
main:
00000:  enterblock depth 0 {a: 0}
00003:  nop
00004:  nullblockchain             <-- trap goes here
00005:  newinit 3
00008:  enterblock depth 2 {x: 0}
00011:  getgname "e"
00014:  iter 1
00016:  goto 36 (+20)
00019:  trace 0
00022:  iternext 1
00024:  setlocal 2
00027:  pop
00028:  newarray 0
00032:  endinit
00033:  arraypush 1
00036:  moreiter
00037:  ifne 19 (-18)
00040:  enditer
00041:  leaveblock 1
00046:  endinit
00047:  nop
00048:  blockchain depth 0 {a: 0}
00051:  setlocal 0
00054:  pop
00055:  getgname "x"
00058:  dup
00059:  getprop "b"
00062:  dup
00063:  pop
00064:  pop
00065:  pop
00066:  leaveblock 1
00071:  stop

Source notes:
 ofs  line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     0 [   0] newline 
  1:    2    16 [  16] xdelta  
  2:    2    16 [   0] if-else  else 11 elseif 21
  5:    2    19 [   3] while    offset 18
  7:    2    27 [   8] xdelta  
  8:    2    27 [   0] decl     offset 2
 10:    2    54 [  27] xdelta  
 11:    2    54 [   0] pcdelta  offset 11
 13:    2    55 [   1] setline  lineno 4
 15:    4    58 [   3] decl     offset 3
 17:    4    65 [   7] decl     offset 0

Exception table:
kind      stack    start      end
 iter         4       19       40

js> trap(f, 4, '')
js> f()
Assertion failure: parent,

Comment 1

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

No need for trap nor any CLI arguments for this testcase:

let([] = eval(), u = let(z = []) {}){}

(gdb) bt
#0  0x001c4944 in JS_Assert (s=0x324f96 "parent", file=0x336110 "/Users/fuzz2/Desktop/jsfunfuzz-dbg-32-mi-73048-47d8748daa90/compilePath/js/src/jsinterp.cpp", ln=5738) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-mi-73048-47d8748daa90/compilePath/js/src/jsutil.cpp:86
#1  0x000e8b42 in js::Interpret () at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-mi-73048-47d8748daa90/compilePath/js/src/jsinterp.cpp:5738
#2  0x000ed2cc in js::RunScript (cx=0x725cf0, script=0x728080, fp=0x1034020) at jsinterp.cpp:613
#3  0x000ed4bc in js::Execute (cx=0x725cf0, script=0x728080, scopeChain=@0x1600010, thisv=@0xbffff550, type=js::EXECUTE_GLOBAL, evalInFrame=0x0, result=0x0) at jsinterp.cpp:908
#4  0x000ed66c in js::ExternalExecute (cx=0x725cf0, script=0x728080, scopeChainArg=@0x1600010, rval=0x0) at jsinterp.cpp:944
#5  0x00029b15 in JS_ExecuteScript (cx=0x725cf0, obj=0x1600010, scriptObj=0x1600150, rval=0x0) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-mi-73048-47d8748daa90/compilePath/js/src/jsapi.cpp:4916
#6  0x00017549 in Process (cx=0x725cf0, obj=0x1600010, filename=0xbffff95c "w46701-reduced.js", forceTTY=false) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-mi-73048-47d8748daa90/compilePath/js/src/shell/js.cpp:482
#7  0x00017fdf in ProcessArgs (cx=0x725cf0, obj=0x1600010, op=0xbffff7a4) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-mi-73048-47d8748daa90/compilePath/js/src/shell/js.cpp:5738
#8  0x00018182 in Shell (cx=0x725cf0, op=0xbffff7a4, envp=0xbffff85c) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-mi-73048-47d8748daa90/compilePath/js/src/shell/js.cpp:5810
#9  0x00018840 in main (argc=2, argv=0xbffff850, envp=0xbffff85c) at /Users/fuzz2/Desktop/jsfunfuzz-dbg-32-mi-73048-47d8748daa90/compilePath/js/src/shell/js.cpp:6009

Comment 2

[Daniel Veditz [:dveditz]]

tracking-8 because this is a regression from something landed in 8.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment 1 is actually a dup of bug 673070. The patch in that bug fixes it and contains tests similar to it.

Comment 0 is different. It demonstrates that if the debugger happens to ask for the scope chain at the one moment when the dynamic scope chain is wrong, we fall over. Here is a test case that fails even without -m.

function f() {
    let (a = let (x = 1) x) {}
}
dis(f);
trap(f, 4, 'evalInFrame(1, "a")');
f();

It also fails if you set the trap at offset 3 rather than 4.

There are two solutions: refuse to set a trap there; or boldly change the bytecode emitted for let-scopes along the lines of bug 646968 comment 2.

I need to sleep on it. Not looking forward to either approach.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v1

The third way is to do further violence to js::GetBlockChain so that it returns a more tactful answer in this rare case.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 551467 [details] [diff] [review]
v1

I just wanted to write down Jason's explanation of this bug in case I forget later.

In the code |let x = E in E'|, we have a hack where this code is emitted:
  enterblock for x
  nullblockchain
  compute E and set x
  blockchain for x
  E'
  leaveblock
All this is necessary because we want E to be evaluated outside of the scope of x. It might seem easier to compute E outside the enterblock, but getting the emitter and decompiler to handle that sounds like a lot of work.

The problem is that the debugger is asking for the scope chain after the enterblock and before the nullblockchain. So we end up returning a block object, which is wrong. Jason's patch forces us to treat the enterblock/nullblockchain pair atomically: if you try to ask for the block chain in between, it skips over the nullblockchain and gives you the correct answer for after that op.

Comment 6

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/28f58b7bc69e

Comment 7

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/28f58b7bc69e

Comment 8

[Jason Orendorff [:jorendorff]]

mbrubeck says this coincided with an 8.7% Dromaeo regression, so I'm backing it out. It was first noticed in mozilla-inbound, so I'm backing it out there:

hg.mozilla.org/integration/mozilla-inbound/rev/913cc99c6bae

Comment 9

[Luke Wagner [:luke]]

Some more details: it quite distinct on WINNT5.1 and 6.1, but Linux, OSX are unchanged.

Comment 10

[Luke Wagner [:luke]]

Also: only for "Dromaeo(SunSpider)", not "SunSpider".

Comment 11

[Luke Wagner [:luke]]

The backout did not fix the regression; assuming I'm reading the graphs right (boy howdy graphs-new.mozilla.org is nicer than than the old one!) I'll reland tomorrow.

Comment 12

[Luke Wagner [:luke]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/431405059a69

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/431405059a69

Comment 14

[u279076]

qa- as there is nothing to do for QA to verify this fix and it appears this is covered in testsuite -- please reset to qa+ if this is not the case.

Comment 15

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

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.