"Assertion failure: parent,"

VERIFIED FIXED in Firefox 8

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: gkw, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla8
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox8+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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,
Whiteboard: js-triage-needed
(Reporter)

Comment 1

6 years ago
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
Summary: JM: "Assertion failure: parent," with trap → "Assertion failure: parent,"
tracking-8 because this is a regression from something landed in 8.
status-firefox8: --- → affected
tracking-firefox8: --- → +
(Assignee)

Updated

6 years ago
Assignee: general → jorendorff
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
Attachment #551467 - Flags: review?(wmccloskey)
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.
Attachment #551467 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 6

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/28f58b7bc69e
Whiteboard: js-triage-needed → [inbound]
Merged:
http://hg.mozilla.org/mozilla-central/rev/28f58b7bc69e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [inbound]
(Assignee)

Comment 8

6 years ago
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
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 9

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

Comment 10

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

Comment 11

6 years ago
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

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

Updated

6 years ago
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/431405059a69
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8

Updated

6 years ago
status-firefox8: affected → fixed
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.
Whiteboard: [qa-]
(Reporter)

Comment 15

5 years ago
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.