808481 - "Assertion failure: lifetime->entry == uint32_t(entryTarget - outerScript->code),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

mjitChunkLimit(35)
eval("\
    0;\
    L: switch(5) {\
        case 1: c:break\
    }\
    while(a);\
")

asserts js debug shell on m-c changeset 2937fd8e35a1 with -a at Assertion failure: lifetime->entry == uint32_t(entryTarget - outerScript->code),

Assuming related to bug 747226. s-s and assuming sec-critical because similar previously-found assertions were also s-s.


===

Due to skipped revisions, the first bad revision could be any of:
changeset:   106120:300ac3d58291
user:        David Anderson
date:        Thu Apr 19 15:02:11 2012 -0700
summary:     Enable JITs and type inference by default, and give Ion first chance (bug 724751, r=jandem).

changeset:   106121:de015aff650d
user:        David Anderson
date:        Thu Apr 19 15:02:47 2012 -0700
summary:     Enable IonMonkey in the browser, and introduce an about:config option to pref it off (bug 745390, r=dmandelin).

changeset:   106122:9e64f779b611
user:        David Anderson
date:        Thu Apr 19 16:57:58 2012 -0700
summary:     Disable TypeScript::SetScope (bug 747226, r=bhackett).

changeset:   106123:bc1833f2111e
user:        Christian Holler
date:        Mon Apr 23 17:32:32 2012 +0200
summary:     Bug 747902 - Add --ion, -n and -m flags back for compatibility

Comment 1

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

Likely a WONTFIX for 16 since it's too late, nominating for the others.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

David - assigning to you since you're likely the culprit if law of averages are to be believed.  Is there an obvious backout here that could return us to a known state? We're going to build on beta 5 tomorrow so the clock's ticking the 17 release.

Comment 3

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

jandem / bhackett were also working on similar assertions before - see bug 781859 and its various dupes.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Wontfixing for 17, if someone has a low-risk fix that can be verified and uplifted before Monday's last beta going to build, please nominate.

Comment 5

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

(In reply to Lukas Blakk [:lsblakk] from comment #4)
> Wontfixing for 17, if someone has a low-risk fix that can be verified and
> uplifted before Monday's last beta going to build, please nominate.

If it's wontfix for 17, it should at least be tracking for 17 esr.

Comment 6

[Christian Holler (:decoder)]

Just found this as well, assuming it's the same thing (32 bit debug build, also only reproduces with -a):

mjitChunkLimit(10);
for (var i = 0; i < 20; i++)
function f0(p0) {
    var v0;
    if (i)
        'bar';
    while (v0 & v0)
        break;
}
f0(0);

Comment 7

[Josh Aas]

David - thoughts on this bug?

Comment 8

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

Sorry, I missed the earlier ping on this bug. Law of average blaming me is definitely fair :) but this one is deep in JM+TI territory. There's no obvious backout. 

Brian or Jan, could you take a look?

Comment 9

[Josh Aas]

Assigning to Jan, who said he'd talk to Brian about this as a next step.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: patch

Bug in the state machine used for picking chunk boundaries.  This code doesn't want to start new chunks after jumps which target their fallthrough opcode, but it also wants to start new chunks at the goto before a for or while loop.  When these goals conflict it ends up using a chunk boundary at the first opcode in the loop, effectively treating it like a do-while loop during compilation.  This might cause problems with the LoopState analysis, not sure.  The attached patch fixes this situation by making the state machine even more horrible.

Comment 11

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

(just a drive-by comment that the patch will likely need sec-approval since the issue is present on the branches)

Comment 12

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

It will need sec-approval because it is a sec-critical bug, per policy.

Comment 13

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

There's a questionaire in the comments which appears, that one needs to fill out when nominating for sec-approval:

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Comment 14

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

Gary, when they set sec-approval to ?, the comment field gets auto-populated with that.

To be clear, if this *didn't* affect branches (We'd never shipped it to anything outside trunk), this could just be checked in without approval.

Comment 15

[Brian Hackett [Laid off!]]

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Not at all.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

Back to Fx 12

If not all supported branches, which bug introduced the flaw?

bug 706914

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backporting is trivial.

How likely is this patch to cause regressions; how much testing does it need?

Not likely at all.

Comment 16

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

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

Looks good. Sec-approval+. We should have branch patches made.

Comment 17

[Alex Keybl [:akeybl]]

Please nominate for branch approval as soon as possible (aurora/beta/esr17). Thanks!

Comment 18

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcf9d812533

Comment 19

[Brian Hackett [Laid off!]]

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

[Approval Request Comment]
See comment 15.

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ffcf9d812533

Comment 21

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27b107941342
https://hg.mozilla.org/releases/mozilla-beta/rev/6f53a5061508
https://hg.mozilla.org/releases/mozilla-esr17/rev/fee2b552544a