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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

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;
}

Comment 1

[Andreas Gal :gal]

Paging david. Not again.

Comment 2

[Robert Sayre]

which branch does this?

Comment 3

[Jason Orendorff [:jorendorff]]

tracemonkey tip.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: tests

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;
}

Comment 5

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

Attachment: proposed fix

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.

Comment 6

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

This is rare, but it should probably block because we can tag pointers incorrectly.

Comment 7

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

Attachment: refactored

sayrer noted that this logic should probably be localized to one function.

Comment 8

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

Attachment: use typemap everywhere. simpler

Using Queue in this part of LeaveTree does not seem to be a performance loss.

Comment 9

[Andreas Gal :gal]

I would prefer the minimal fix for 1.9.1.

Comment 10

[Robert Sayre]

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.

Comment 11

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

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.

Comment 12

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

http://hg.mozilla.org/tracemonkey/rev/3a1ab9f19b66

Comment 13

[Daniel Veditz [:dveditz]]

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?

Comment 14

[Andreas Gal :gal]

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.

Comment 15

[Brendan Eich [:brendan]]

Propagate the cleanup. It will help not only here but future merges.

/be

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3a1ab9f19b66

Comment 17

[Daniel Veditz [:dveditz]]

Not yet landed for 1.9.2, this misses the 1.9.1.4 release

Comment 18

[Robert Sayre]

(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?

Comment 19

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e9b65c3da0a6

Comment 20

[Samuel Sidler (old account; do not CC)]

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.

Comment 21

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

This does not apply. I will get a 1.9.1-applicable patch here tomorrow.

Comment 22

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

Attachment: branch patch

Comment 23

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

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.

Comment 24

[Andreas Gal :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.

Comment 25

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

(In reply to comment #24)

For posterity, IRL conclusion was to not bother doing this since it's a branch patch.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 410572 [details] [diff] [review]
branch patch

Approved for 1.9.1.6, a=dveditz for release-drivers

Comment 27

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cad82a9d57c7

Comment 28

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

Bob, can you verify this for 1.9.1?

Comment 29

[Bob Clary [:bc] (inactive)]

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

Comment 30

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

Since this is blocking on 1.9.1, we need someone to look at this during the next day or two.

Comment 31

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

Bob, you might want to check on 1.9.2 as well since it is blocking there.

Comment 32

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

(In reply to comment #29)

Are you sure? I checked out a clean 1.9.1 tree and couldn't reproduce the problem.

Comment 33

[Bob Clary [:bc] (inactive)]

did you run the shell with -j ?

1.9.2/1.9.3 look clean.

Comment 34

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

Yup, I tried both saving as a file and copy+paste into interactive shell.

Comment 35

[Bob Clary [:bc] (inactive)]

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.

Comment 36

[Christian Holler (:decoder)]

Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.