Closed Bug 470364 Opened 11 years ago Closed 11 years ago

Make [[DefaultValue]] implementation *consistently* non-conforming until we can fix it to behave correctly in both JIT, non-JIT

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: Waldo, Unassigned)

References

Details

(Keywords: fixed1.9.1, testcase, Whiteboard: [good first bug])

Attachments

(1 file)

js> var o = { valueOf: function() { if (arguments.length !== 0) throw "unexpected arguments!  arg1 type=" + typeof arguments[0] + ", value=" + arguments[0]; return 2; } };
js> o + 3
uncaught exception: unexpected arguments!  arg1 type=string, value=undefined

See ECMA-262 8.6.2.6, which specifies how valueOf and toString are called inside [[DefaultValue]].

Most or all of the imacro implementations have this bug as well, which is how I discovered it.
JS_ConvertStub -> js_TryValueOf passes an array consisting of the type hint as a string.

It looks like all our own objects just use JS_ConvertStub; JSAPI docs[0] are a bit self-contradictory in that they say it implements the default conversion behavior of native JS Objects and then proceed to describe the non-standard behavior demonstrated above.  Should we write new convert methods for the built-in objects (need two to handle the difference in behavior of Date when not passed a hint) to address this compatibly and leave JS_ConvertStub unaltered?  (Perhaps even expose a JS_NewConvertStub used on everything but Dates to allow embedders [including Mozilla eventually?] to switch to the standardized behavior?)

This is all easy enough to do, but it's clearly low priority.  Anyone interested in learning about SpiderMonkey could take this as a simple way to wet his feet in the code without having to know much initially, once decisions are reached on the questions raised in the previous paragraph.

0. https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_PropertyStub
Assignee: jwalden+bmo → general
Whiteboard: [good first bug]
Status: ASSIGNED → NEW
Fixing this will also involve modifications to obj_valueOf and its associated trace-info macro, Object_p_valueOf, and perhaps a change or two elsewhere that I'm not aware of yet.  Of course, tests would also be good, probably reasoning backward from [[DefaultValue]] in ECMA-262 to anything that calls it in the spec and back from there to uses at-a-distance via actual JS operations (rather than spec-internal functions/operators).
date_valueOf and date_valueOf_tn would also need to be changed, because they rely on the presence/absence of the string argument to handle [[DefaultValue]]() as specified in ECMA-262.  That further will require specializing imacros which implement ToPrimitive(o) in the case where o is a Date, because such imacros are implementations of [[DefaultValue]]().
Flags: blocking1.9.1?
Summary: Implementation of [[DefaultValue]] incorrectly calls valueOf, toString with an argument → TM: Implementation of [[DefaultValue]] incorrectly calls valueOf, toString with an argument
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
This is an ancient, ancient bug -- it predates ES1's finalization. Is it really a blocker? wanted1.9.1+ at most, I think.

/be
I think I agree.  I doubt overriding valueOf or toString with a function that actually takes any arguments is particularly common, and I think it's a bit late in the cycle to change this in any case.
...or rather, overriding with a function whose behavior changes based on whether it's being called with any arguments or not.
OK, I misread. I thought imacros were doing it in more cases than a non-JIT build would. blocking- assuming there is no change from the interpreter.
Flags: blocking1.9.1+ → blocking1.9.1-
This adds the non-standard type argument back for JSOP_NEG and JSOP_POS when they're being traced upon objects with custom valueOf functions; I didn't have it originally because I thought we could fix the interpreter implementations, but it turned out to be a bigger problem than was really worth fixing at the moment.
Attachment #356881 - Flags: review?(brendan)
Attachment #356881 - Flags: review?(brendan) → review+
Flags: wanted1.9.1+
Comment 9 got backed out, but http://hg.mozilla.org/tracemonkey/rev/4a65cc2a2872 relanded it.
http://hg.mozilla.org/mozilla-central/rev/4a65cc2a2872
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Keywords: testcase
http://hg.mozilla.org/mozilla-central/rev/98b2ab987dd7
/cvsroot/mozilla/js/tests/ecma_3/Object/8.6.2.6-002.js,v  <--  8.6.2.6-002.js
initial revision: 1.1

fyi: this test checks the original bug and currently fails. It does not test the checkin and is not to be used to reopen this bug.
Flags: in-testsuite+
Flags: in-litmus-
Blocks: 476624
Adjusting summary to reflect what was actually don here; the first several comments were what I wanted to do, but then I hijacked my own bug to revert to bad behavior and reaped the fruits of my mislabors.  Bug 476624 now tracks what this bug originally tracked.
Summary: TM: Implementation of [[DefaultValue]] incorrectly calls valueOf, toString with an argument → Make [[DefaultValue]] implementation *consistently* non-conforming until we can fix it to behave correctly in both JIT, non-JIT
test ecma_3/Object/8.6.2.6-002.js covers Bug 476624, not this bug.
Keywords: testcase
Flags: in-testsuite+ → in-testsuite?
Keywords: testcase
You need to log in before you can comment on or make changes to this bug.