User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release) Build ID: 20140216133306 Steps to reproduce: In my project, I want to compute RMSD by JavaScript. I have observed that my RMSD method returns a wrong result after several cycles. I have these troubles with firefox-24.3.0esr, if javascript.options.baselinejit.content is enabled. On the other hand, I did not have these troubles with firefox-27.0.1. The HTML file with the JavaScript code is attached. The code has been generated by GWT originally, I have tried making it shorter. Actual results: The method is called in a for-loop for the same input data, and wrong results are obtained in some iterations. An error result is reported by a alert message ("error for i = "). Expected results: I expect that the results of the method will be the same in each iteration. Only a message "done" should by showed.

We should bisect this probably.

Progression window(m-i) Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/057cd362da69 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130909111349 Fixed: http://hg.mozilla.org/integration/mozilla-inbound/rev/a1bd3bb5a0ba Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130909111650 Progression Pushlog:( http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=057cd362da69&tochange=a1bd3bb5a0ba Fixed by: a1bd3bb5a0ba Hannes Verschore — Bug 909717: IonBuilder: Introduce typed typebarriers, r=jandem

Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/b31bfbb2bdc4 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130403 Firefox/23.0 ID:20130403083520 Bad: http://hg.mozilla.org/mozilla-central/rev/b5cb88ccd907 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130403 Firefox/23.0 ID:20130403084327 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b31bfbb2bdc4&tochange=b5cb88ccd907 Triggered by: Merge baseline compiler...

If memory serves well, I don't think bug 909717 would/can solve a bug. I think the changes probably hide the original problem... So we might want to look closer about what is happening...

If I replace call "new Vector_0()" by "new Vector_1(0.0, 0.0, 0.0)", then the code works fine on firefox/24.0. It also works, if I add "this.x_0 = 0.0; this.y_0 = 0.0; this.z_0 = z_0;" into Vector_0. Thus, the reported problem is observed, if vector constructed by Vector_0 uses x_0, y_0 and z_0 from its prototype.

I'm looking into this now (ESR24 build). There are 2 different MCreateThisWithProto instructions, each is followed by its own MBox and somehow GVN thinks the two MBox instructions have the same operand. Will know more soon.

Ugh, MCreateThisWithProto inherits congruentTo from MBinaryInstruction :(

Jakub, thank you for the report. This is an older IonMonkey bug, surprised it doesn't break more stuff. This needs some refactoring to avoid similar bugs in the future, but I'll post simpler patches for aurora/beta/ESR.

Although this testcase does not fail on Firefox 26+, the problem is still there. Updating flags.

The main problem is that MBinaryInstruction and friends shouldn't override congruentTo: it means all instructions that inherit from them will get a default implementation that you probably don't want or expect. In this case MCreateThisWithProto but there are others I think. So this patch: (*) Removes MTernaryInstruction::congruentTo and MQuaternaryInstruction::congruentTo. Updated derived classes to call congruentIfOperandsEqual. (*) Renames MBinaryInstruction::congruentTo to binaryCongruentTo. It's like congruentIfOperandsEqual, but also handles commutative instructions. (*) Fixes some classes that inherit from these. For effectful instructions, we don't need to override congruentTo (congruentIfOperandsEqual will always return false). For the other classes, I made sure they override congruentTo. (*) Fixes some congruentTo issues I noticed.

The previous patch breaks the testBullet.js jit-test, because we mark MBinaryBitwiseInstruction as commutative when we specialize it as int32. With the patch binaryCongruentTo takes advantage of this (we didn't do this before) and things break because shift instructions are not commutative. The fix is to only mark MBinaryBitwiseInstruction as commutative if it's a BitOr, BitAnd or BitXor.

Comment on attachment 8394345 [details] [diff] [review] Patch Review of attachment 8394345 [details] [diff] [review]: ----------------------------------------------------------------- I think you have forgotten to add congruentTo to the following ops: MAtan2 MHypot MPow MConcat MSetArgumentsObjectArg (maybe?) MToId MStringSplit There are even more that inherited MBinaryInstruction::congruentTo, but I don't think they need congruentTo. ::: js/src/jit/MIR.cpp @@ +170,5 @@ > > if (isEffectful() || ins->isEffectful()) > return false; > > + MOZ_ASSERT(numOperands() == ins->numOperands()); I think we might want to keep the numOperands() != ins->numOperands() check. E.g. for a MInstruction inheriting MVariadicInstruction. We currently don't have such a case, but I don't think it makes sense to disable it. At least I don't see a reason for it.

(In reply to Hannes Verschore [:h4writer] from comment #12) > I think you have forgotten to add congruentTo to the following ops: > MAtan2 > MHypot > MPow > MConcat These all override congruentTo. > MSetArgumentsObjectArg (maybe?) > MToId These are effectful so we'll never eliminate them. > MStringSplit For this one we don't want congruentTo, it can cause bugs like the one here: two similar MStringSplit instructions have to return different arrays. > I think we might want to keep the numOperands() != ins->numOperands() check. > E.g. for a MInstruction inheriting MVariadicInstruction. We currently don't > have such a case, but I don't think it makes sense to disable it. At least I > don't see a reason for it. OK I can add it back.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f11569eb914c https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7088a1f186 Will prepare simpler patches for other branches.

(In reply to Jan de Mooij [:jandem] from comment #13) > These all override congruentTo. Indeed. My bad.

https://hg.mozilla.org/mozilla-central/rev/f11569eb914c https://hg.mozilla.org/mozilla-central/rev/ba7088a1f186

Very simple and safe fix for branches.

Comment on attachment 8396713 [details] [diff] [review] Patch for branches Review of attachment 8396713 [details] [diff] [review]: ----------------------------------------------------------------- Nice to have this fixed for these two issues. Definitely correct. Now since we now this is a bigger issue, which metric did you use to decide on what was important to uplift and what not?

(In reply to Hannes Verschore [:h4writer] from comment #18) > Now since we now this is a bigger issue, which metric did you use to decide > on what was important to uplift and what not? I fixed all instructions that inherit from BinaryInstruction and have this problem. The m-c patch is better because it prevents similar bugs in the future, but the other patch should fix the problems on other branches.

Comment on attachment 8396713 [details] [diff] [review] Patch for branches [Approval Request Comment] Bug caused by (feature/regressing bug #): Older Ion bugs. User impact if declined: Broken websites, like the one reported here Testing completed (on m-c, etc.): A similar patch landed on m-c more than a week ago. Risk to taking this patch (and alternatives if risky): Very low. String or IDL/UUID changes made by this patch: None.

https://hg.mozilla.org/releases/mozilla-aurora/rev/76754e144866 https://hg.mozilla.org/releases/mozilla-beta/rev/81285325c7db

Reproduced the initial issue on Firefox 24RC and verified as fixed on Firefox 29 beta 7, latest Aurora and latest Nightly using Ubuntu 13.04 64bit.

Jan - Given that this issue was reported on ESR 24.3.0 and your branch patches are very small, what is your opinion on the risk associated with backporting this fix to ESR24?

(In reply to Lawrence Mandel [:lmandel] from comment #23) > Jan - Given that this issue was reported on ESR 24.3.0 and your branch > patches are very small, what is your opinion on the risk associated with > backporting this fix to ESR24? Oh. This can easily get nominated for esr24 uplift. Should be very low risk.

Sounds good. If you can get a branch patch created or the esr24 approval flag set to nom for the existing patch I'll follow up with approval. If we can get this landed this week it can get into ESR 24.6.0.

Comment on attachment 8396713 [details] [diff] [review] Patch for branches Lawrence, I've been told in the past that we're only fixing security issues on ESR24 at this point. Requesting approval in case that's not true. [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: Correctness issues. User impact if declined: Broken websites. Fix Landed on Version: 31, backported to 29 Risk to taking this patch (and alternatives if risky): Very low. String or UUID changes made by this patch: None.

Comment on attachment 8396713 [details] [diff] [review] Patch for branches Review of attachment 8396713 [details] [diff] [review]: ----------------------------------------------------------------- ESR approval granted. Please land this week. The general rule is that ESR is for high severity security fixes. However, we do have the ability to ship other fixes, specifically correctness fixes. In this case the fix was reported by an ESR user on the ESR channel and is a low risk fix.

I dropped the MStringSplit hunk since bug 935016 only landed on 28+. https://hg.mozilla.org/releases/mozilla-esr24/rev/4a99027b4a02