Binary operations not evaluating operands in correct order

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Waldo, Assigned: brendan)

Tracking

({verified1.9.0.1})

Trunk
verified1.9.0.1
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [RC2-], URL)

Attachments

(1 attachment)

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

Comment 1

10 years ago
Created attachment 320918 [details] [diff] [review]
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
Attachment #320918 - Flags: review?(igor)

Updated

10 years ago
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.
(Assignee)

Updated

10 years ago
Whiteboard: [RC2?]

Updated

10 years ago
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)

Updated

10 years ago
Assignee: general → brendan
(Assignee)

Comment 5

10 years ago
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
(Assignee)

Comment 6

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 7

9 years ago
/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-

Comment 8

9 years ago
verified 1.9.0/trunk linux/mac/win modulo division operators.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.1 → verified1.9.0.1
(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.