Closed Bug 433672 Opened 13 years ago Closed 12 years ago

Binary operations not evaluating operands in correct order

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: Waldo, Assigned: brendan)

References

()

Details

(Keywords: verified1.9.0.1, Whiteboard: [RC2-])

Attachments

(1 file)

js> var alert = print;
js> ({valueOf: function() { alert('vO left'); throw 5; }}) - ({valueOf: function() { alert('vO right'); return 3; }})
vO right
vO left
uncaught exception: 5
js> ({valueOf: function() { alert('vO left'); throw 5; }}) - ({valueOf: function() { alert('vO right'); throw 7; }})
vO right
uncaught exception: 7

I think we have to flip these two lines to get the order of the checks right, which as I read 11.6.1 is left to right:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/jsinterp.c&rev=3.497&mark=3690-3691#3685

With that fixed, I think we'd pass every test at the URL.  Maybe.  I'm a bit tired right now to fully trust myself on that.

I kind of think that if we have the RCs to do it this should go in 1.9.  If I've diagnosed problem and fix correctly, this is about as well-understood as they come.
Attached patch fixSplinter Review
Another 13-year-old bug from version 1 -- Waldo, thanks for reporting this!

Could someone who is around full time please take this bug? Thanks,

/be
Attachment #320918 - Flags: review?(igor)
Attachment #320918 - Flags: review?(igor) → review+
I merely did the easy part of skimming #webkit; olliej did the hard part of writing the test.  I'll pass along the thanks next time he's in there.

Didn't we have this bug with the shift operators at one time?  I could have sworn that bug was to adjust behavior of one odd case to conform with everything else, not just the one odd case we happened to hit that we discovered was wrong.

I still think this is RC2-fodder, but I'll not step on your toes by putting [RC2?] in the status whiteboard.
Whiteboard: [RC2?]
Whiteboard: [RC2?] → [RC2-]
Brendan: should we be considering this for 3.0.1? If so, please request approval1.9.0.1? What's the risk like - maybe it should wait for Gecko 1.9.1?
Comment on attachment 320918 [details] [diff] [review]
fix

Approving for 1.9.0.1; risk is epsilon, good easy standards and interop fix.
Attachment #320918 - Flags: approval1.9.0.1+
Assignee: general → brendan
Fixed on cvs trunk for 1.9.0.1:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.499; previous revision: 3.498
done

/be
Keywords: fixed1.9.0.1
Fixed on mozilla-central:

changeset:   15514:30543128f056
tag:         tip
user:        Brendan Eich <brendan@mozilla.org>
date:        Tue Jun 24 17:28:22 2008 -0700
summary:     Fix 433672, r=igor, a=shaver.

/be
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
/cvsroot/mozilla/js/tests/ecma_3/Operators/order-01.js,v  <--  order-01.js
initial revision: 1.1

changeset:   16463:6ec5f36dcd62

I'm not sure how to test (?:), (,) as they do not appear to call valueOf as I would expect from the ecma3 spec. 

the "division" operators fail. see Bug 449509
Flags: in-testsuite+
Flags: in-litmus-
verified 1.9.0/trunk linux/mac/win modulo division operators.
Status: RESOLVED → VERIFIED
(In reply to comment #7)
> I'm not sure how to test (?:), (,) as they do not appear to call valueOf as I
> would expect from the ecma3 spec. 

(belatedly) ECMA3 doesn't specify a call to valueOf there.  GetValue isn't the operation that calls valueOf.  ToNumber and ToPrimitive do that.
You need to log in before you can comment on or make changes to this bug.