751320 - JavaScript variable intermittently not assigned value after operation (Firefox; stand-alone test case)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tyler Horton]

Attachment: Test page to reproduce FF12 bug of intermittent variable assignment failure.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

Exercised functionality in my company's webapp after customers complained about data corruption.  Their complaints were reproducible only in Firefox 12 (which they had recently upgraded to) *without* any Firebug panels activated.


Actual results:

Essentially, the values returned by a function called repeatedly in a for loop began to yield different results for the same output.  Please see the attached stand-alone test case.


Expected results:

The stand-alone test case will execute correctly in Firefox 11 (or lower), Safari, and Chrome.  Two alerts will appear indicating "START test" and "END test" with no failure notices.  However, opening the test page in FF12 (without Firebug panel activated) will indicate a "test FAILED..." condition in an alert.

While a malicious attack may be difficult to construct from this test case, I am still marking this as a security bug since the outcome of such a basic failure is so unpredictable.

Comment 1

[Tyler Horton]

This test page fails on FF12 on Windows and MacOS X (and probably others).

Comment 2

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

I confirm that the behavior is different between 11 and 12.

Comment 3

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f63a99195987&tochange=798b00a6fe29 is when the problem first appears.

Almost certainly a regression from bug 704387.

Comment 4

[Luke Wagner [:luke]]

Hopefully this is the same underlying bug as bug 748044.

Comment 5

[Daniel Veditz [:dveditz]]

Tracking Firefox 13 as a web-compat regression

Comment 6

[Daniel Veditz [:dveditz]]

I don't think this introduces any general vulnerability in Firefox. It may be a security issue for some rare web application, but we'll make better progress on a fix if more people can see the bug.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: patch

Thanks for the great testcase!

This is a miscompilation where if a variable changes from int to double within a switch statement, then after jumping to case labels after the variable turns to a double the compiled code may treat the variable as a double even though it is still represented as an integer.  The fix adds checks to ensure the variable has the correct representation when jumping to such case labels.

Comment 8

[Luke Wagner [:luke]]

Awesome, can we dup bug 748044 to this?

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #8)
> Awesome, can we dup bug 748044 to this?

I think so, but I need to see more of the malfunctioning switch in bug 748044 to confirm.

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c801fd1b52

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9c801fd1b52

Comment 12

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

Verified fixed in nightly trunk build from changset: http://hg.mozilla.org/mozilla-central/rev/448f554f6acb.

Comment 13

[Alex Keybl [:akeybl]]

Is this bug sufficiently low risk or does it have significant enough of a web compatibility impact that we should land now on FF13? If so, please nominate prior to 5/21.

Comment 14

[Brian Hackett [Laid off!]]

Comment on attachment 620722 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Some websites may behave incorrectly (confusing integers and doubles, no s-s).
Risk to taking this patch (and alternatives if risky): Very low.

Comment 15

[David Mandelin [:dmandelin]]

(In reply to Brian Hackett (:bhackett) from comment #14)
> Comment on attachment 620722 [details] [diff] [review]
> patch
> 
> [Approval Request Comment]
> User impact if declined: Some websites may behave incorrectly (confusing
> integers and doubles, no s-s).
> Risk to taking this patch (and alternatives if risky): Very low.

A little extra background for the approval requests:
 - The bug did ship in Fx12, so if 13 goes out with the bug, at least we are not regressing further.
 - Bug 748044, breakage of a Mandreel game, is believed to a be a dup of this bug, so that's an extra user impact if declined. Rumor has it that Mandreel worked around but I have no way to directly verify that.

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 620722 [details] [diff] [review]
patch

[Triage Comment]
Given the low-risk evaluation and confirmed user impact, approving for Aurora 14 and Beta 13.

Comment 17

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9fd1222af3b1

(Beta has been closed since yesterday and I can't find anyone on IRC to open it up.)

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Beta is open again, please go ahead with landing.

Comment 19

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6758d4042459

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Target milestone tracks when it landed on m-c. The tracking flags track uplifts.