781859 - Assertion failure: lifetime && lifetime->head == uint32_t(head - outerScript->code) && lifetime->entry == uint32_t(entryTarget - outerScript->code), at methodjit/LoopState.cpp:80

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on mozilla-central revision b5ae446888f5 (options -m -n -a):


function e() {
    try {} catch (e) {
    return (actual = "FAIL");
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
      a.x + a.x + a.x + a.x + a.x + a.x + a.x + a.x
    }
    while (t) continue;
}
e();


I believe this is something different than bug 741110 and bug 770089 since the test looks entirely different and doesn't depend on mjitChunkLimit. S-s due to previous bugs with that assertion being security-relevant.

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   87695:f4e955f78de9
user:        Jeff Walden
date:        Fri Feb 03 18:53:29 2012 -0800
summary:     Bug 720316 - Use uint32_t indexes for JOF_ATOM opcodes.  r=jorendorff

Comment 2

[Christian Holler (:decoder)]

Waldo or Jason, mind taking a look at this?

Comment 3

[Jason Orendorff [:jorendorff]]

sure.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Provisionally calling this sec-critical. Always welcome thoughts from those closer to the code.

Comment 5

[Jason Orendorff [:jorendorff]]

Full assertion is:
    JS_ASSERT(lifetime &&
              lifetime->head == uint32_t(head - outerScript->code) &&
              lifetime->entry == uint32_t(entryTarget - outerScript->code));

lifetime->entry                   is 1507 (points to JSOP_LOOPHEAD)
entryTarget - outerScript->code   is 1506 (points to JSOP_LOOPENTRY)

Comment 6

[Jeff Walden [:Waldo]]

Bug 720316 is likely a bogus dependency, just as the original dependency in bug 770089 was, and this possibly goes back to chunked compilation notwithstanding the absence of mjitChunkLimit from the testcase.  (In effect every script has an implicit |mjitChunkLimit(#)| call in it, actually, for some value of # that I don't know offhand.  This just manages to crash with whatever that default limit is.)  Therefore this might well be a dup of one of the other two bugs, although there's no actual evidence beyond the common assertion -- and, come to see it, this having a try-catch-then-loop structure in common with the testcase for bug 741110.  If there's a dup relationship, and there may well not be one, I bet it's with that bug.

Comment 7

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

Synchronizing flags with bug 741110 and bug 770089.

Comment 8

[Jeff Walden [:Waldo]]

I'm sure critsmash is going to be interested in this bug (and the other bugs with similar-looking assertions) today, so...

Right before he left bhackett said something about thinking the assertion was wrong, but he didn't give any explanation at the time, and I haven't been able to verify that one way or the other yet.  So the short answer is that I don't know yet what's up here.  (And obviously that doesn't apply to the one similar-looking bug where an out-and-out crash was reported.)

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Jeff, it helps (really). Hopefully work on the other two bugs helps illuminate this one.

Comment 10

[Jeff Walden [:Waldo]]

billm and I looked at this for half an hour or so on Thursday afternoon.  Neither of us understands chunked compilation, so we're pretty much stumbling in the dark on this.  And I have been told chunked compilation -- as done in JM, at least -- doesn't really have things like invariants.  So there's apparently not really a good way to understand how chunked compilation works.

dvander says jandem is the person best positioned to understand JM chunked compilation -- maybe he'd be able to figure this out quickly?  'cause it's going to take me awhile to do this, if I'm to be the one to do it.  :-\

Comment 11

[Jan de Mooij [:jandem]]

Yeah this is a chunked compilation issue. While-loops have the following bytecode structure:

00000:  goto 6 (+6)
00005:  loophead
--- body ---
00006:  loopentry
--- condition ---
00008:  ifne 5 (-3)
00013:  stop

When splitting the script in chunks, we try to ensure the initial GOTO is in the same chunk as the LOOPHEAD following it. The way this works is if we are at op X, we check if X + 2 is a LOOPHEAD and if so start a new chunk at X + 1 (the GOTO).

This works fine in most cases, except if the op at offset X, before the GOTO, is unreachable/dead. Unreachable ops are skipped so we don't run the check and hence it's possible to start a chunk at the LOOPHEAD, and the GOTO is in a different chunk.

I tried to fix this in MakeJITScript, but it's not easy. A much simpler fix I think is to emit a JSOP_NOP right before the JSOP_GOTO. This is nice anyway because it matches what we emit for other loops.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 1: Add SRC_DO

Once while loops also have a JSOP_NOP, we need a way to distinguish while and do-while loops when we see a JSOP_NOP with SRC_WHILE source note.

This patch adds a separate SRC_DO source note. It also slightly simplifies things since it can store two offsets, instead of using a second source note for the next op.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 2: Emit JSOP_NOP at the top of while loops

Add JSOP_NOP before the initial JSOP_GOTO, and move the source note to it.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 3: Add shell function to test the decompiler

Adds decompileFunctionDeprecated to the shell, I used this to test the decompiler changes in the other patches.

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Jason asked some good questions, which made me realize it's actually simpler to allow a chunk boundary between the GOTO and LOOPHEAD, and fix the few places in the compiler where it assumes otherwise.

Comment 18

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f31bcbcdbf4

Comment 19

[Ed Morley [:emorley]]

Backed out for an approx 50% failure rate of Android-no-ion mochitest 8, of the form:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15774715&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=15774814&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65404c2b69c4

Comment 20

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

jandem, do you mind adding the testcase from bug 797163 as well, when you fix up and reland the patch?

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #20)
> jandem, do you mind adding the testcase from bug 797163 as well, when you
> fix up and reland the patch?

Sure I will add it, thanks for the tests!

Comment 22

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

Definitely too late for 16.

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 667095 [details] [diff] [review]
Patch v2

gkw or decoder, could one of you fuzz this patch for a while with --no-ion? Reducing the M8 correctness bug will take a lot of time, so it would be very useful if the fuzzers could find a shell testcase in a few minutes. Thanks!

Comment 24

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

> gkw or decoder, could one of you fuzz this patch for a while with --no-ion?

Don't hold your hopes too high from me, I've fuzzed for an hour or two but nothing has showed up yet.

Comment 25

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

Comment on attachment 667095 [details] [diff] [review]
Patch v2

I didn't see anything here after a day of fuzzing with the patch.

Comment 26

[Brian Hackett [Laid off!]]

Attachment: patch

This patch seems to work ok, it doesn't skip unreachable opcodes while constructing chunks but just treats them as non-branching.  Passes jit-tests -mna and the three testcases in the comment 17 patch (will include those testcases when pushing).  I'm not sure what's wrong with the comment 17 patch but the code handling loop heads wrt OSR, LICM and loop-carried registers is pretty delicate (why the chunked compilation patch originally opted to not insert chunk boundaries in weird places).

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 674069 [details] [diff] [review]
patch

Review of attachment 674069 [details] [diff] [review]:
-----------------------------------------------------------------

Nice minimal fix, thanks.

Comment 28

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d60a2c574f4

Comment 29

[Al Billings [:abillings - ex-MoCo]]

This is a sec-high bug. It should have been nominated with the sec-approval flag on the patch and received explicit approval for landing. This is now the policy for all sec-high and sec-critical issues. See posts to dev.planning on this and https://wiki.mozilla.org/Security/Bug_Approval_Process.

Comment 30

[Alex Keybl [:akeybl]]

(In reply to Brian Hackett (:bhackett) [mostly gone until mid-November] from comment #28)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0d60a2c574f4

If this is low risk enough for uplift to FF17, please nominate for uplift asap.

Comment 31

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 93cc1ee94291).

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d60a2c574f4

Comment 33

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 34

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 674069 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): chunked compilation (old)
User impact if declined: potential security bug
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): This patch only changes how we ignore unreachable JavaScript code. Low-risk.
String or UUID changes made by this patch:

Comment 35

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

Needs landing for aurora/beta.

Comment 36

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/releases/mozilla-aurora/rev/59c1b8fe4249
https://hg.mozilla.org/releases/mozilla-beta/rev/33a74acf43f9

Comment 37

[u279076]

Is there somewhere I can look at test results so I can mark this verified for Firefox 17, 18, and 19?

Comment 38

[Matt Wobensmith [:mwobensmith][:matt:]]

+1 to Anthony.

QA would love to regress this bug on affected branches. A link to test results OR a working test case that we can run would suffice. Thank you.