Closed Bug 510518 Opened 13 years ago Closed 13 years ago

"Assertion failure: isInt32(*p), at ../jstracer.cpp:2587" with nested trees

Categories

(Core :: JavaScript Engine, defect, P1)

Other Branch
defect

Tracking

()

VERIFIED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .6+
status1.9.1 --- .6-fixed

People

(Reporter: jorendorff, Assigned: dvander)

Details

(Keywords: testcase, verified1.9.1, verified1.9.2, Whiteboard: [sg:critical?])

Attachments

(3 files, 2 obsolete files)

var arr = [
    [0, 0, 0, 0, 0], // inner tree compiles first path
    [0, 0, 0, 0, 0],
    [0, 0, 0, 0, 0], // outer tree compiles
    [0, 0, 0, 0, 0],
    [0, 0, 0, 0, 0],
    [1, 0, 0, 0, 0], // inner tree compiles different path, assinging x = ""
                     // outer tree extends (different inner tree exit)
    [1, 0, 0, 0, 0],
    [1, 0, 0, 0, 0],
    [1, 0, 0, 0, 0], // Assertion failure: isInt32(*p), at ../jstracer.cpp:2587
    [1, 0, 0, 0, 0],
    ];

var x = 0;

for (var i = 0; i < 10; i++) {
    var b = arr[i];
    for (var j = 0; j < 5; j++) {
        if (b[0])
            x = "";
    }
    x = 0;
}
Group: core-security
Paging david. Not again.
which branch does this?
tracemonkey tip.
Attached patch testsSplinter Review
Here are some tests to include in the fix.

One is a test for bug 502714 (already fixed). The other is a reduced version of the test in comment 0:

var x = 0;
for (var i = 0; i < 20; i++) {
    for (var j = 0; j < 5; j++) {
        if (i > 5)
            x = "";
    }
    x = 0;
}
Attached patch proposed fix (obsolete) — Splinter Review
bug 502604 and bug 502714 addressed the underlying problem here. Unfortunately AttemptToExtendTree was still assuming that the outer global typemap would always have more globals than the inner typemap. This is not true. LeaveTree() and the tree call mechanism already knew this and computed the global typemap length correctly.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #395445 - Flags: review?(gal)
This is rare, but it should probably block because we can tag pointers incorrectly.
blocking1.9.1: --- → ?
Flags: blocking1.9.2?
Attachment #395445 - Flags: review?(gal) → review+
Attachment #395445 - Flags: approval1.9.1.3?
Attached patch refactored (obsolete) — Splinter Review
sayrer noted that this logic should probably be localized to one function.
Attachment #395445 - Attachment is obsolete: true
Attachment #395445 - Flags: approval1.9.1.3?
Using Queue in this part of LeaveTree does not seem to be a performance loss.
Attachment #395469 - Attachment is obsolete: true
Attachment #395482 - Flags: review?(gal)
I would prefer the minimal fix for 1.9.1.
Attachment #395482 - Flags: review?(gal) → review+
Comment on attachment 395482 [details] [diff] [review]
use typemap everywhere.  simpler

> globalTypeMap = typeMap.data();

that is still not very palatable, but it is most likely not a security bug.
Attachment #395482 - Flags: review?(sayrer) → review+
Flags: blocking1.9.2? → blocking1.9.2+
It's safe - though if we're worried about it breaking if code moves around in the future, I can add some followup comments/assertions.
Blocking 1.9.1.4 for now, but gated on when the fix lands for 1.9.2 so might be the next one.

> I would prefer the minimal fix for 1.9.1.

Generally I'd agree, but in the case of complicated code there's an argument for being consistent across branches too. What's better in this case?
blocking1.9.1: ? → .4+
Keywords: testcase
Whiteboard: [sg:critical?]
The difference is only code reshuffling/cleanup. No functional difference. If you are ok with the extra risk of the slight code movement, I don't mind either patch on branch. They both do the same thing.
Propagate the cleanup. It will help not only here but future merges.

/be
http://hg.mozilla.org/mozilla-central/rev/3a1ab9f19b66
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Not yet landed for 1.9.2, this misses the 1.9.1.4 release
blocking1.9.1: .4+ → .5+
(In reply to comment #17)
> Not yet landed for 1.9.2, this misses the 1.9.1.4 release

when did landing on an unreleased branch become a requirement?
Priority: -- → P1
Does this patch apply to 1.9.1? If not, can you please attach one that does? Code freeze for 1.9.1.6 is set for November 10 at 11:59pm PDT and this blocks that release.

Please request approval on a 1.9.1-applicable patch.
This does not apply. I will get a 1.9.1-applicable patch here tomorrow.
Attached patch branch patchSplinter Review
Attachment #410572 - Flags: review?(sayrer)
Attachment #410572 - Flags: approval1.9.1.6?
Comment on attachment 410572 [details] [diff] [review]
branch patch

This should be identical to the other patch with a few exceptions:
 * Some inline helpers are missing so the code is a little more verbose.
 * JSTraceType is uint8
 * Queue needed the max==0 support.
Attachment #410572 - Flags: review?(sayrer) → review?(gal)
Comment on attachment 410572 [details] [diff] [review]
branch patch

Please set max to 0 in the default constructor to avoid malloc-ing in hot paths when we don't actually use the typemap.
Attachment #410572 - Flags: review?(gal) → review+
(In reply to comment #24)

For posterity, IRL conclusion was to not bother doing this since it's a branch patch.
Comment on attachment 410572 [details] [diff] [review]
branch patch

Approved for 1.9.1.6, a=dveditz for release-drivers
Attachment #410572 - Flags: approval1.9.1.6? → approval1.9.1.6+
Bob, can you verify this for 1.9.1?
Nope. It's not fixed on mac at least.

$ Darwin_DBG.OBJ/js -j
js> var arr = [
    [0, 0, 0, 0, 0], // inner tree compiles first path
    [0, 0, 0, 0, 0],
    [0, 0, 0, 0, 0], // outer tree compiles
    [0, 0, 0, 0, 0],
    [0, 0, 0, 0, 0],
    [1, 0, 0, 0, 0], // inner tree compiles different path, assinging x = ""
                     // outer tree extends (different inner tree exit)
    [1, 0, 0, 0, 0],
    [1, 0, 0, 0, 0],
    [1, 0, 0, 0, 0], // Assertion failure: isInt32(*p), at ../jstracer.cpp:2587
    [1, 0, 0, 0, 0],
    ];

var x = 0;

for (var i = 0; i < 10; i++) {
    var b = arr[i];
    for (var j = 0; j < 5; j++) {
        if (b[0])
            x = "";
    }
    x = 0;
}js> js> js> js> 
Assertion failure: isInt32(*p), at ../jstracer.cpp:2121
Trace/BPT trap
Since this is blocking on 1.9.1, we need someone to look at this during the next day or two.
Bob, you might want to check on 1.9.2 as well since it is blocking there.
(In reply to comment #29)

Are you sure? I checked out a clean 1.9.1 tree and couldn't reproduce the problem.
did you run the shell with -j ?

1.9.2/1.9.3 look clean.
Yup, I tried both saving as a file and copy+paste into interactive shell.
verified 1.9.1,1.9.2,1.9.3 shell on mac os x. not sure what was up with my previous brain fart, but after an rm and re-build it works fine.
Status: RESOLVED → VERIFIED
Group: core-security
Flags: wanted1.9.0.x-
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.