Last Comment Bug 672804 - "Assertion failure: parent,"
: "Assertion failure: parent,"
Status: VERIFIED FIXED
[qa-]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla8
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on:
Blocks: jsfunfuzz 646968
  Show dependency treegraph
 
Reported: 2011-07-20 08:02 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-12-13 17:06 PST (History)
11 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
v1 (3.30 KB, patch)
2011-08-08 08:49 PDT, Jason Orendorff [:jorendorff]
wmccloskey: review+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2011-07-20 08:02:44 PDT
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] 2011-07-20 08:59:18 PDT
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] 2011-07-21 13:44:27 PDT
tracking-8 because this is a regression from something landed in 8.
Comment 3 Jason Orendorff [:jorendorff] 2011-07-25 23:18:16 PDT
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] 2011-08-08 08:49:21 PDT
Created attachment 551467 [details] [diff] [review]
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 (:billm) 2011-08-08 10:34:35 PDT
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] 2011-08-10 15:50:13 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/28f58b7bc69e
Comment 7 Mounir Lamouri (:mounir) 2011-08-11 04:22:53 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/28f58b7bc69e
Comment 8 Jason Orendorff [:jorendorff] 2011-08-11 15:01:34 PDT
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] 2011-08-11 15:09:50 PDT
Some more details: it quite distinct on WINNT5.1 and 6.1, but Linux, OSX are unchanged.
Comment 10 Luke Wagner [:luke] 2011-08-11 15:39:42 PDT
Also: only for "Dromaeo(SunSpider)", not "SunSpider".
Comment 11 Luke Wagner [:luke] 2011-08-12 00:23:34 PDT
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 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-14 04:53:36 PDT
http://hg.mozilla.org/mozilla-central/rev/431405059a69
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 10:37:12 PDT
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] 2012-12-13 17:06:08 PST
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.

Note You need to log in before you can comment on or make changes to this bug.