912264 - Assertion failure: false (MOZ_ASSUME_UNREACHABLE(Modified registers between VM call and OsiPoint)), at jit/shared/CodeGenerator-shared.cpp:521

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision d54e0cce6c17 (run with --fuzzing-safe --ion-eager):


enableOsiPointRegisterChecks();
gczeal(4);
with({});
eval('for (var x in this) {}');
evaluate("\
var sText = 's';\
for (var i = 0; i < 250000; -i)\
    sText += 'a\\n';\
", { noScriptRval : true });

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Jan de Mooij [:jandem]]

decoder, can we run autoBisect on this testcase?

Comment 3

[Christian Holler (:decoder)]

We can try it :)

Comment 4

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/4be6c333853a
user:        Nicolas Pierron
date:        Thu Aug 29 10:20:28 2013 -0700
summary:     Bug 909544 - IonMonkey: Check VM Call sanity in visit functions. r=jandem

This iteration took 342.132 seconds to run.

Comment 5

[Jan de Mooij [:jandem]]

needinfo? per comment 4 :)

Comment 6

[Daniel Veditz [:dveditz]]

Is this a crash that could happen in web content, or is this a crash in the extra checks added by enableOsiPointRegisterChecks?

Comment 7

[Christian Holler (:decoder)]

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

Comment 8

[Christian Holler (:decoder)]

I suspect this isn't fixed. I also vaguely remember that the test was sometimes not reproducing properly... so let's just try this.

Comment 9

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision e42374f83509).
JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c8dffd55eeef
user:        Nicholas D. Matsakis
date:        Fri Aug 30 07:14:30 2013 -0400
summary:     Bug 910777 - Add JSCLASS_IMPLEMENTS_BARRIERS to the Binary Data classes with custom trace r=sfink

This iteration took 407.602 seconds to run.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Christian Holler (:decoder) from comment #9)
> The first good revision is:
> changeset:   http://hg.mozilla.org/mozilla-central/rev/c8dffd55eeef

This sounds unrelated.

Comment 11

[Nicolas B. Pierron [:nbp]]

Based on the comment 4, I we should consider this bug as sec-critical until proven safe.

What surprised me is that I did not spot any invalid use of callVM last time I audited the CodeGen for the try&catch patches.

Comment 12

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

Any luck?

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to David Bolter [:davidb] from comment #12)
> Any luck?

Not yet, I did not investigated more since the last comment.
I will keep the need-info as a reminder. Thanks for the ping. :)

Comment 14

[Nicolas B. Pierron [:nbp]]

Ok, this was hard to follow and I am still missing a few pieces of the puzzle, but it seems that:
  1/ We do a concatenation with the callVM path.
  2? Raise the interrupt check.
  3/ Jump to the OutOfLine path of the interrupt check of the loop, without doing the reset of the checkRegs flags.
  4/ Fail on the OSI Point of the Interrupt check.

The assertion seems to be raised because we did not executed the reset of the checkRegs flags which is done before the execution of the instruction, as we jump directly to the out-of-line path.

I will try to understand from where we jump to the oolInterruptCheck and if there is no risk involved in this process.  The goal would be to determine if the OSI Point of the concatenation is executed before we jump to the oolInterruptCheck (which is likely), in which case there is no risk and this is just a really tricky false alarm.

(Keep the needinfo on my-self until I get a precise idea of what is going on)

Comment 15

[Nicolas B. Pierron [:nbp]]

… why does bugzilla reset flags that I did not changed …

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: Reset osi-regs flags before the implicit interrupt check.

(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> I will try to understand from where we jump to the oolInterruptCheck and if
> there is no risk involved in this process.  The goal would be to determine
> if the OSI Point of the concatenation is executed before we jump to the
> oolInterruptCheck (which is likely), in which case there is no risk and this
> is just a really tricky false alarm.

Ok, this is what is happening here. :)
The backedge of the loop jumps directly into the OOL path of the implicit interrupt check, and thus skip the reset added by generateBody.

Comment 17

[Nicolas B. Pierron [:nbp]]

I remove the sec-critical rating based on comment 16.

Comment 18

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

Is this no longer a security issue and we can unhide it then?

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Al Billings [:abillings] from comment #18)
> Is this no longer a security issue and we can unhide it then?

I confident that this is no longer a security issue, but I prefer to wait for the review in case I missed something.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 811362 [details] [diff] [review]
Reset osi-regs flags before the implicit interrupt check.

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

Good catch.

Comment 21

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e91a399223 r=jandem

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/22e91a399223

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

please nominate for Aurora uplift.

Comment 24

[Nicolas B. Pierron [:nbp]]

(In reply to [e-mail] [:lsblakk] from comment #23)
> please nominate for Aurora uplift.

I am happy to uplift it but I do not how this would be useful except for fuzzing.
Is this the goal of this tracking flag?

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Nicolas B. Pierron [:nbp] from comment #24)
> (In reply to [e-mail] [:lsblakk] from comment #23)
> > please nominate for Aurora uplift.
> 
> I am happy to uplift it but I do not how this would be useful except for
> fuzzing.
> Is this the goal of this tracking flag?

Yes, if it's a regression (and it's marked as such) then we will usually track for the channels affected.  If this is low risk to uplift, we might as well get this onto FF26 branch (which is about to be Beta today).

Comment 26

[Nicolas B. Pierron [:nbp]]

(In reply to [e-mail] [:lsblakk] from comment #25)
> (In reply to Nicolas B. Pierron [:nbp] from comment #24)
> > (In reply to [e-mail] [:lsblakk] from comment #23)
> > > please nominate for Aurora uplift.
> > 
> > I am happy to uplift it but I do not how this would be useful except for
> > fuzzing.

s/how/think/

> > Is this the goal of this tracking flag?
> 
> Yes, if it's a regression (and it's marked as such) then we will usually
> track for the channels affected.  If this is low risk to uplift, we might as
> well get this onto FF26 branch (which is about to be Beta today).

This "regression" is not visible in any way from users in any way. This patch just sanitize the assertion.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

removing tracking as per comment 26, if this isn't visible to users and there's no other reason to uplift to beta let's just let it ride the trains.