501275 - TM: Crash [@ nanojit::Assembler::nPatchBranch]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

for (let i = 0; i < 5; ++i) { i / (i * i); }

Crash [@ nanojit::Assembler::nPatchBranch]

Comment 1

[Jesse Ruderman]

Does not affect the 1.9.1 branch.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to bug 498549 :

The first bad revision is:
changeset:   28992:434d3673d304
user:        Andreas Gal
date:        Wed Jun 17 08:06:21 2009 +0100
summary:     If the result of a demoted multiplication is 0, must undemote or we lose -0 (498549, r=dvander).

Comment 3

[Andreas Gal :gal]

Confirmed.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This bug is hurting the bug 465479 fuzzer significantly on all platforms.

Comment 5

[Dave Townsend [:mossop]]

This can also be seen by visiting the stats page of a pro flickr account: http://crash-stats.mozilla.com/report/index/d7c28c82-ba85-4e5a-a07c-c757b2090718

Comment 6

[Andreas Gal :gal]

Attachment: Skip guard records attached to a side exit that ended up being eliminated

Comment 7

[Andreas Gal :gal]

498549 is a red herring. It allows the testcase to compile more efficiently than before, and contains a side exit that has one of its guards eliminated by the forward filter. We do link it in sideexit->guards though, and when a 2nd trace is attached to the side exit, we try to patch guardRecord->jmp, which is NULL (guard was eliminated and never emitted). Hilarity ensues. 

This can be triggered prior to 498549, but not with the attached test case or anything similar to it. Even if, its a "safe" always-NULL crash, and probably really rare edge case.

The attached test-case only triggers on m-c and TM, not 1.9.1. I will not close this bug, but mark it [sg? dos], and ask for 3.5.2 approval.

Comment 8

[Andreas Gal :gal]

The patch is very low risk. This can go onto branch with minimum baking.

Comment 9

[Andreas Gal :gal]

I reviewed all places where we emit guards and the new code in TM and m-c might be the only place that can actually trigger this (conceptually other things can trigger it, but we seem to never emit any other guards that can become redundant). So 1.9.1 is likely not affected, but the patch is so low risk and my analysis might be wrong, so definitively still want this in 3.5.2

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 389355 [details] [diff] [review]
Skip guard records attached to a side exit that ended up being eliminated

a1912=beltzner, I bet your analysis is right, but better safe than sorry. Watch out for that merge!

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This needs to first be checked in to tracemonkey branch.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/ac1757deafa7

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ac1757deafa7

Comment 14

[Robert Sayre]

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

Comment 15

[Bob Clary [:bc] (inactive)]

the referenced test never failed on 1.9.1. it still doesn't on mac at least.

Comment 16

[u279076]

Based on comment 15, is it safe to mark this verified1.9.1?

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

Comment 18

[Bob Clary [:bc] (inactive)]

js/src/trace-test.js

Comment 19

[Bob Clary [:bc] (inactive)]

testEliminatedGuardWithinAnchor
v 1.9.3, 1.9.2