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

Paging david. Not again.

which branch does this?

tracemonkey tip.

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

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.

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

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

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

I would prefer the minimal fix for 1.9.1.

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.

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?

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

Not yet landed for 1.9.2, this misses the 1.9.1.4 release

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

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.

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 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.

(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

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.

Filter on qa-project-auto-change: Bug in removed tracer code, setting in-testsuite- flag.