Last Comment Bug 781393 - Value overwriting during VM stack scanning can overwrite copied values
: Value overwriting during VM stack scanning can overwrite copied values
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Bill McCloskey (:billm)
: general
Mentors:
Depends on: 782782
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-08 17:31 PDT by Bill McCloskey (:billm)
Modified: 2012-10-09 11:34 PDT (History)
3 users (show)
wmccloskey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
affected
fixed
fixed
unaffected


Attachments
patch (5.51 KB, patch)
2012-08-09 14:53 PDT, Bill McCloskey (:billm)
bhackett1024: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Review

Description Bill McCloskey (:billm) 2012-08-08 17:31:20 PDT
Here's a test case. With the patch in bug 781390, it passes with no options and fails with -m -n -a.

gczeal(4,1);
function check(o)
{
    print(o);
    assertEq(o.b, 3);
}
function f()
{
    for (var i=0; i<3; i++) {
	var o = {b: 3};
	check(o);
    }
}
f();
Comment 1 Bill McCloskey (:billm) 2012-08-09 14:53:54 PDT
Created attachment 650688 [details] [diff] [review]
patch

This is the simpler fix we discussed.
Comment 3 Bill McCloskey (:billm) 2012-08-15 11:08:04 PDT
Comment on attachment 650688 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 750834
User impact if declined: JavaScript code could generate incorrect results.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-08-15 18:45:11 PDT
https://hg.mozilla.org/mozilla-central/rev/a5bda3083952
Comment 5 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-16 11:12:38 PDT
Comment on attachment 650688 [details] [diff] [review]
patch

What's the reach of this to users?  What kind of behaviour will be experienced on the web.  This was never nominated for tracking and it's late in FF15's beta cycle so I'm not likely to take this there at this point, but will consider uplift to Aurora if there's a serious user-facing problem here otherwise this can ride the trains.
Comment 6 Bill McCloskey (:billm) 2012-08-16 14:11:30 PDT
It's hard to quantify how much this could affect people. On rare occasions, when running JavaScript code, we will overwrite a variable with an incorrect value. This can happen either in JS code from web pages or in our own chrome JS code. I see an argument for not landing it on beta, but I really think this should land on Aurora.
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-17 10:27:32 PDT
Comment on attachment 650688 [details] [diff] [review]
patch

Works for me, we have more time to shake out any regressions if this goes to Aurora now and if all goes well it will be out to our GA in 7 weeks.
Comment 8 Bill McCloskey (:billm) 2012-08-19 14:09:05 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/63c85d3cb319
Comment 9 Bill McCloskey (:billm) 2012-08-19 15:37:38 PDT
I forgot that this bug kinda depends on bug 782782 in the sense that the patch here exposes that bug on tryserver. Anyway, I landed that change here:

https://hg.mozilla.org/releases/mozilla-aurora/rev/95a9ef9dfc3d
Comment 10 Ioana (away) 2012-10-09 05:54:11 PDT
(In reply to Bill McCloskey (:billm) from comment #6)
> It's hard to quantify how much this could affect people. On rare occasions,
> when running JavaScript code, we will overwrite a variable with an incorrect
> value. This can happen either in JS code from web pages or in our own chrome
> JS code. I see an argument for not landing it on beta, but I really think
> this should land on Aurora.

Bill, is there a way QA can trigger one of those rare occasions and verify this bug?
Comment 11 Bill McCloskey (:billm) 2012-10-09 11:30:11 PDT
Well, the test case attached in the patch will trigger it. That runs on tinderbox, so I don't think there's anything for QA to do here.
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-09 11:31:23 PDT
(In reply to Bill McCloskey (:billm) from comment #11)
> Well, the test case attached in the patch will trigger it. That runs on
> tinderbox, so I don't think there's anything for QA to do here.

Does that qualify for in-testsuite+?
Comment 13 Bill McCloskey (:billm) 2012-10-09 11:34:42 PDT
Yes. I always forget to set that flag.

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