Last Comment Bug 510518 - "Assertion failure: isInt32(*p), at ../jstracer.cpp:2587" with nested trees
: "Assertion failure: isInt32(*p), at ../jstracer.cpp:2587" with nested trees
Status: VERIFIED FIXED
: [sg:critical?]
: testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: JavaScript Engine
: Other Branch
: All All
: P1 normal (vote)
: ---
Assigned To: David Anderson [:dvander]
: general
:
:
:
  Show dependency treegraph
 
Reported: 2009-08-14 08:46 PDT by Jason Orendorff [:jorendorff]
Modified: 2010-01-08 11:40 PST (History)
8 users (show)
sayrer: blocking1.9.2+
dveditz: wanted1.9.0.x-
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1-fixed
  .6+
  .6-fixed


Attachments
tests (1.31 KB, patch)
2009-08-14 11:02 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
proposed fix (2.64 KB, patch)
2009-08-19 16:19 PDT, David Anderson [:dvander]
gal: review+
Details | Diff | Splinter Review
refactored (6.68 KB, patch)
2009-08-19 17:48 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
use typemap everywhere. simpler (6.17 KB, patch)
2009-08-19 18:46 PDT, David Anderson [:dvander]
gal: review+
sayrer: review+
Details | Diff | Splinter Review
branch patch (6.44 KB, patch)
2009-11-05 11:53 PST, David Anderson [:dvander]
gal: review+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review

Summon comment box

Description Jason Orendorff [:jorendorff] 2009-08-14 08:46:47 PDT
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 2009-08-14 08:48:37 PDT
Paging david. Not again.
Comment 2 Robert Sayre 2009-08-14 09:35:49 PDT
which branch does this?
Comment 3 Jason Orendorff [:jorendorff] 2009-08-14 10:53:22 PDT
tracemonkey tip.
Comment 4 Jason Orendorff [:jorendorff] 2009-08-14 11:02:36 PDT
Created attachment 394532 [details] [review]
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] 2009-08-19 16:19:53 PDT
Created attachment 395445 [details] [review]
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] 2009-08-19 16:20:58 PDT
This is rare, but it should probably block because we can tag pointers incorrectly.
Comment 7 David Anderson [:dvander] 2009-08-19 17:48:27 PDT
Created attachment 395469 [details] [review]
refactored

sayrer noted that this logic should probably be localized to one function.
Comment 8 David Anderson [:dvander] 2009-08-19 18:46:38 PDT
Created attachment 395482 [details] [review]
use typemap everywhere.  simpler

Using Queue in this part of LeaveTree does not seem to be a performance loss.
Comment 9 Andreas Gal :gal 2009-08-19 18:57:26 PDT
I would prefer the minimal fix for 1.9.1.
Comment 10 Robert Sayre 2009-08-19 19:45:49 PDT
Comment on attachment 395482 [details] [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] 2009-08-20 13:08:36 PDT
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] 2009-08-20 13:10:13 PDT
http://hg.mozilla.org/tracemonkey/rev/3a1ab9f19b66
Comment 13 Daniel Veditz 2009-08-21 10:59:46 PDT
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 2009-08-21 11:23:09 PDT
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] 2009-08-21 14:57:33 PDT
Propagate the cleanup. It will help not only here but future merges.

/be
Comment 17 Daniel Veditz 2009-09-21 15:22:03 PDT
Not yet landed for 1.9.2, this misses the 1.9.1.4 release
Comment 18 Robert Sayre 2009-09-21 15:25:45 PDT
(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 20 Samuel Sidler (old account; do not CC) 2009-11-04 17:16:44 PST
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] 2009-11-04 17:25:02 PST
This does not apply. I will get a 1.9.1-applicable patch here tomorrow.
Comment 22 David Anderson [:dvander] 2009-11-05 11:53:27 PST
Created attachment 410572 [details] [review]
branch patch
Comment 23 David Anderson [:dvander] 2009-11-05 11:55:24 PST
Comment on attachment 410572 [details] [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 2009-11-05 12:34:30 PST
Comment on attachment 410572 [details] [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] 2009-11-05 16:40:55 PST
(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 2009-11-06 11:35:45 PST
Comment on attachment 410572 [details] [review]
branch patch

Approved for 1.9.1.6, a=dveditz for release-drivers
Comment 28 Al Billings [:abillings] 2009-11-20 11:30:19 PST
Bob, can you verify this for 1.9.1?
Comment 29 Bob Clary [:bc:] 2009-11-23 11:49:09 PST
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] 2009-11-23 12:02:34 PST
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] 2009-11-23 12:02:56 PST
Bob, you might want to check on 1.9.2 as well since it is blocking there.
Comment 32 David Anderson [:dvander] 2009-11-23 12:11:34 PST
(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:] 2009-11-23 12:18:21 PST
did you run the shell with -j ?

1.9.2/1.9.3 look clean.
Comment 34 David Anderson [:dvander] 2009-11-23 12:21:22 PST
Yup, I tried both saving as a file and copy+paste into interactive shell.
Comment 35 Bob Clary [:bc:] 2009-11-23 13:12:32 PST
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.

Note You need to log in before you can comment on or make changes to this bug.