862100 - IonMonkey: Assertion failure: ins->type() == MIRType_Value, at ion/MIR.h:1833

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 261d6997d1d1 (run with --ion-eager):


function TestCase(n, d, e, a) {}
function reportCompare (expected, actual, description) {
  new TestCase("", description, expected, actual);
}
new TestCase( "", "", 0, Number(new Number()) );
reportCompare(true, true);
evaluate("\
function TestCase(n, d, e, a) {}\
test_negation(-2147483648, 2147483648);\
test_negation(2147483647, -2147483647);\
function test_negation(value, expected)\
    reportCompare(expected, '', '-(' + value + ') == ' + expected);\
");

Comment 1

[Christian Holler (:decoder)]

S-s because previous asserts like this we're s-s sometimes.

Comment 2

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   122585:437c955ff06d
user:        Nicolas B. Pierron
date:        Wed Jan 30 07:41:01 2013 -0800
summary:     Bug 796114 - Inline with type-checked arguments. r=h4writer

This iteration took 0.776 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo from Nicolas based on comment 2 :)

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 738254 [details] [diff] [review]
Patch

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

Ok, for this patch, after all this function would be removed with the removal of the analysis types.

::: js/src/ion/IonBuilder.cpp
@@ +3123,5 @@
> +                        MInstruction *toDouble = MUnbox::New(ins, MIRType_Double, MUnbox::Fallible);
> +                        current->add(toDouble);
> +                        ins = toDouble;
> +                    }
> +                    JS_ASSERT(ins->type() == MIRType_Double);

nit: Sounds like this should be added as part of the previous if condition which is checking if:

    callerType == JSVAL_TYPE_DOUBLE || ins->type() == MIRType_Double

Comment 6

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61e661489460

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61e661489460

Comment 8

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 738254 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 796114


User impact if declined: 
Crashes around zero with special scripts, not sure if possible to attack in browser

Testing completed (on m-c, etc.):
m-i: 1 day, m-c: 1 hour

Risk to taking this patch (and alternatives if risky): 
Almost no risk

String or IDL/UUID changes made by this patch:
/

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 738254 [details] [diff] [review]
Patch

When landing this, we probably want to land bug 863261 at the same time. I will request approval again, when other bug lands. (Should happen quickly)

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Patch

This is the version that got checked in. Carrying r+ over.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 796114

User impact if declined: 
Possible crashes during compilation on special crafted scripts. That's the reason it took so long to find this issue.

Testing completed (on m-c, etc.): 
m-c: already a week, I think

Risk to taking this patch (and alternatives if risky): 
Very low. This change make sure we don't unbox already unboxed double. Note that there is a known issue with this patch. I will only push when bug 863755 is approved too.

String or IDL/UUID changes made by this patch:
/

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 740920 [details] [diff] [review]
Patch

Approving along with bug 863755 given the minimal risk here, but would be good to understand what the security rating fort his bug is.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/078b54f5e7c7

Comment 14

[u279076]

Marking status-firefox23:verified based on comment 8.