"Assertion failure: isInt32(*p)" with simple testcase

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
JavaScript Engine
P1
critical
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: dvander)

Tracking

(Blocks: 1 bug, {assertion, fixed1.9.1, testcase})

Trunk
mozilla1.9.1
x86
Mac OS X
assertion, fixed1.9.1, testcase
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
for (var i = 0; i < 4; ++i) {
    var x = 1;
    for (var j = 0; j < 3; ++j) {
        if (j > 1) {
            x = {};
        }
    }
}
print(x);

Incorrectly gives 2885984 when it should give "[object Object]".  If I change the inner loop bound from 3 to 4, I get an assertion instead:

Assertion failure: isInt32(*p), at ../jstracer.cpp:2031

This bug affects both TM tip and 1.9.1 branch.  The testcase is simple enough that I'd expect web sites to hit it.
Flags: blocking1.9.1?

Comment 1

9 years ago
This is really not fun. I don't understand why none of our tests catches this stuff.
Assignee: general → gal
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Reduced testcase:

function f() {
    x = 1;
    for (var j = 0; j < 3; ++j) {
        if (j > 1) {
            x = {};
        }
    }
}

f();
f();
f();

print(x);
Created attachment 382165 [details] [diff] [review]
fix

Silly bug. Side exit coalescing code wasn't checking that both exits had the same number of global types.
Assignee: gal → dvander
Status: NEW → ASSIGNED
Attachment #382165 - Flags: review?(gal)

Comment 4

9 years ago
Comment on attachment 382165 [details] [diff] [review]
fix

Possible fix but I think it would be better to extend the destination map if it matches but is too short.

Updated

9 years ago
Attachment #382165 - Flags: review?(gal) → review+

Comment 5

9 years ago
Comment on attachment 382165 [details] [diff] [review]
fix

r+ with the rationale that memcmp without length check is wrong for sure and this is a minimal fix. will discuss further whether this causes duplicate traces which could be avoided by growing an existing trace.
Are we memcmp'ing off the end of an allocation even with the patch?

/be
(In reply to comment #6)
> Are we memcmp'ing off the end of an allocation even with the patch?
> 
> /be

Since stack slots should always match, it should be okay.
Would asserting that be vacuous? Not obviously but maybe I am missing context.

/be

Comment 9

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2b57dcfdb13d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.