433672 - Binary operations not evaluating operands in correct order

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

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

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

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 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 320918 [details] [diff] [review]
fix

Approving for 1.9.0.1; risk is epsilon, good easy standards and interop fix.

Comment 5

[Brendan Eich [: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

Comment 6

[Brendan Eich [:brendan]]

Fixed on mozilla-central:

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

/be

Comment 7

[Bob Clary [:bc] (inactive)]

/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

Comment 8

[Bob Clary [:bc] (inactive)]

verified 1.9.0/trunk linux/mac/win modulo division operators.

Comment 9

[Jason Orendorff [:jorendorff]]

(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.