Last Comment Bug 416354 - Missing SAVE_SP_AND_PC in JSOP_NEG
: Missing SAVE_SP_AND_PC in JSOP_NEG
Status: VERIFIED FIXED
[sg:critical?]
: verified1.8.1.13
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
P1 normal (vote)
: mozilla1.9beta4
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-08 07:27 PST by Igor Bukanov
Modified: 2008-03-29 16:07 PDT (History)
4 users (show)
brendan: blocking1.9+
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next-
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (1.14 KB, patch)
2008-02-08 07:30 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.13+
brendan: approval1.9+
Details | Diff | Splinter Review
js1_5/extensions/regress-416354.js (2.56 KB, text/plain)
2008-02-22 03:18 PST, Bob Clary [:bc:]
no flags Details

Description User image Igor Bukanov 2008-02-08 07:27:44 PST
JSOP_NEG in the interpreter calls js_NewNumberValue without calling SAVE_SP_AND_PC when the top of the stack is a double value. This leads to a GC hazard as the following example demonstrates:

~/m/ff/mozilla/js/src $ cat ~/m/y.js
function f(a, b, c)
{
    return (-a) * ((-b) * (-c));
}

var expect = f(1.5, 1.25, 1.125);
gczeal(2);
var actual = f(1.5, 1.25, 1.125);
if (actual !== expect)
    throw "GC hazard, expect="+expect+" actual="+actual;
~/m/ff/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/m/y.js
uncaught exception: GC hazard, expect=-2.109375 actual=-1.58203125
Comment 1 User image Igor Bukanov 2008-02-08 07:30:10 PST
Created attachment 302122 [details] [diff] [review]
v1

The fix makes sure that SAVE_SP_AND_PC is called at the right moment.
Comment 2 User image Igor Bukanov 2008-02-08 07:31:31 PST
Asking for blocking flags as this is a GC hazard on the trunk and 1.8.1 branch.
Comment 4 User image Igor Bukanov 2008-02-15 12:51:05 PST
Comment on attachment 302122 [details] [diff] [review]
v1

The patch applies to the 181 branch as is.
Comment 5 User image Daniel Veditz [:dveditz] 2008-02-20 11:25:05 PST
Comment on attachment 302122 [details] [diff] [review]
v1

approved for 1.8.1.13, a=dveditz for release-drivers
Comment 6 User image Bob Clary [:bc:] 2008-02-22 03:18:12 PST
Created attachment 304950 [details]
js1_5/extensions/regress-416354.js
Comment 7 User image Bob Clary [:bc:] 2008-02-26 08:08:15 PST
v
Comment 8 User image Igor Bukanov 2008-02-29 13:22:29 PST
I checked in the patch from comment 1 to MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.96; previous revision: 3.181.2.95
done
Comment 9 User image Bob Clary [:bc:] 2008-03-17 04:45:35 PDT
v 1.8.1 linux|mac 10.5
Comment 10 User image Alexander Sack 2008-03-23 08:46:12 PDT
igor, what should we do with these SAVE_SP_AND_PC on the 1.8.0 branch?
Comment 11 User image Igor Bukanov 2008-03-23 16:20:14 PDT
(In reply to comment #10)
> igor, what should we do with these SAVE_SP_AND_PC on the 1.8.0 branch?

The bug does not exist on 1.8.0 branch.
Comment 12 User image Alexander Sack 2008-03-25 09:21:42 PDT
thanks. not a 1.8.0 branch bug.
Comment 13 User image Bob Clary [:bc:] 2008-03-29 16:07:33 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-416354.js,v  <--  regress-416354.js
initial revision: 1.1

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