Last Comment Bug 751320 - JavaScript variable intermittently not assigned value after operation (Firefox; stand-alone test case)
: JavaScript variable intermittently not assigned value after operation (Firefo...
Status: VERIFIED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 12 Branch
: x86 All
: -- critical (vote)
: mozilla15
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
: 748044 (view as bug list)
Depends on: 748044
Blocks: 704387
  Show dependency treegraph
 
Reported: 2012-05-02 12:53 PDT by Tyler Horton
Modified: 2012-05-22 05:32 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
+
fixed
verified
unaffected


Attachments
Test page to reproduce FF12 bug of intermittent variable assignment failure. (889 bytes, text/html)
2012-05-02 12:53 PDT, Tyler Horton
no flags Details
patch (2.62 KB, patch)
2012-05-03 08:55 PDT, Brian Hackett (:bhackett)
dvander: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Tyler Horton 2012-05-02 12:53:14 PDT
Created attachment 620434 [details]
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 2012-05-02 12:55:41 PDT
This test page fails on FF12 on Windows and MacOS X (and probably others).
Comment 2 Al Billings [:abillings] 2012-05-02 13:14:18 PDT
I confirm that the behavior is different between 11 and 12.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-05-02 13:18:45 PDT
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] 2012-05-02 14:05:00 PDT
Hopefully this is the same underlying bug as bug 748044.
Comment 5 Daniel Veditz [:dveditz] 2012-05-02 19:04:15 PDT
Tracking Firefox 13 as a web-compat regression
Comment 6 Daniel Veditz [:dveditz] 2012-05-02 19:06:08 PDT
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 (:bhackett) 2012-05-03 08:55:15 PDT
Created attachment 620722 [details] [diff] [review]
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] 2012-05-03 09:17:05 PDT
Awesome, can we dup bug 748044 to this?
Comment 9 Brian Hackett (:bhackett) 2012-05-03 09:46:38 PDT
(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 (:bhackett) 2012-05-05 07:19:35 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c801fd1b52
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-05-05 20:31:05 PDT
https://hg.mozilla.org/mozilla-central/rev/b9c801fd1b52
Comment 12 Al Billings [:abillings] 2012-05-07 13:08:39 PDT
Verified fixed in nightly trunk build from changset: http://hg.mozilla.org/mozilla-central/rev/448f554f6acb.
Comment 13 Alex Keybl [:akeybl] 2012-05-16 17:16:49 PDT
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 (:bhackett) 2012-05-16 17:26:25 PDT
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] 2012-05-18 08:51:01 PDT
(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] 2012-05-18 15:43:14 PDT
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 (:bhackett) 2012-05-19 15:19:52 PDT
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 2012-05-21 13:58:20 PDT
Beta is open again, please go ahead with landing.
Comment 19 Brian Hackett (:bhackett) 2012-05-21 16:15:18 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/6758d4042459
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-05-21 17:41:36 PDT
Target milestone tracks when it landed on m-c. The tracking flags track uplifts.
Comment 21 Luke Wagner [:luke] 2012-05-22 05:32:12 PDT
*** Bug 748044 has been marked as a duplicate of this bug. ***

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