Last Comment Bug 673070 - "Assertion failure: sharedBlock," with e4x
: "Assertion failure: sharedBlock," with e4x
Status: VERIFIED FIXED
js-triage-needed [qa-]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla8
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz 646968
  Show dependency treegraph
 
Reported: 2011-07-21 02:21 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-10 16:36 PST (History)
9 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
stack (3.05 KB, text/plain)
2011-07-21 02:21 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
v1 (9.48 KB, patch)
2011-07-25 21:15 PDT, Jason Orendorff [:jorendorff]
brendan: review+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2011-07-21 02:21:29 PDT
Created attachment 547348 [details]
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] 2011-07-21 13:45:01 PDT
tracking-8 because this is a regression from something landed in 8.
Comment 2 Jason Orendorff [:jorendorff] 2011-07-22 15:56:02 PDT
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] 2011-07-25 21:15:03 PDT
Created attachment 548361 [details] [diff] [review]
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] 2011-07-27 10:04:47 PDT
Comment on attachment 548361 [details] [diff] [review]
v1

Argh, I shoulda grepped when reviewing the last patch.

/be
Comment 5 Marco Bonardo [::mak] 2011-08-04 03:18:42 PDT
http://hg.mozilla.org/mozilla-central/rev/a73864715b24
Comment 6 Johnathan Nightingale [:johnath] 2011-08-16 14:45:22 PDT
Marking fixed on firefox8 based on checkin two weeks before migration
Comment 7 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 10:41:04 PDT
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) 2013-01-04 13:21:33 PST
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] 2013-01-10 16:36:15 PST
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.

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