673070 - "Assertion failure: sharedBlock," with e4x

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

for (let w in [0]) {
    let([] = ([
        <x/>
    ]), r = <x/>) {}
}

asserts js debug shell on MI changeset 99d121a0f799 without any CLI arguments at Assertion failure: sharedBlock,

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.

Comment 1

[Daniel Veditz [:dveditz]]

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

Comment 2

[Jason Orendorff [:jorendorff]]

Whew.

There are two JSOP_TOXML opcodes in this, at offsets 45 and 59. JSOP_TOXML indirectly calls js::GetScopeChain.

The problem is that the first toxml is (incorrectly) dynamically inside the innermost let scope. In short, everything goes nuts from there.

dis();
00000:  callgname "dis"
00003:  call 0
00006:  popv

for (let w in [0]) {
00007:  enterblock depth 0 {w: 0}
00010:  nop
00011:  nullblockchain
00012:  object [0]
00015:  nop
00016:  blockchain depth 0 {w: 0}
00019:  iter 1
00021:  goto 73 (+52)

    w <- next value
00024:  trace 0
00027:  iternext 1
00029:  setlocal 0
00032:  pop

    let ([] = ([<x/>]),
00033:  enterblock depth 2 {r: 1, : 0}
00036:  newarray 1
00040:  zero
00041:  startxml
00042:  string "<x/>"
00045:  toxml
00046:  initelem
00047:  endinit
00048:  dup
00049:  pop
00050:  pop

        r = <x/>)
00051:  nop
00052:  blockchain depth 0 {w: 0}
00055:  startxml
00056:  string "<x/>"
00059:  toxml
00060:  nop
00061:  blockchain depth 2 {r: 1, : 0}
00064:  setlocal 3
00067:  pop

    {}
00068:  leaveblock 2

}
00073:  moreiter
00074:  ifne 24 (-50)
00077:  enditer
00078:  leaveblock 1
00083:  stop

Might be a straightforward bytecode emitter bug. jsemit is supposed to be using TempPopScope every time it emits code in a let-head, but here there's destructuring assignment and we're not. I must have missed a case.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

...I'm particularly proud of the comments, which almost make it sound like this really isn't that precarious at all.

Comment 4

[Brendan Eich [:brendan]]

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

Argh, I shoulda grepped when reviewing the last patch.

/be

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/a73864715b24

Comment 6

[Johnathan Nightingale [:johnath]]

Marking fixed on firefox8 based on checkin two weeks before migration

Comment 7

[u279076]

qa- as nothing to do for QA fix verification -- please reset to qa+ if there is something for QA to verify.

Comment 8

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-673070-3.js.

Comment 9

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

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.