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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

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

Comment 2

[Jeff Walden [:Waldo]]

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

Comment 3

[Jeff Walden [:Waldo]]

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

Comment 4

[Brendan Eich [:brendan]]

This is an ancient, ancient bug -- it predates ES1's finalization. Is it really a blocker? wanted1.9.1+ at most, I think.

/be

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Jeff Walden [:Waldo]]

...or rather, overriding with a function whose behavior changes based on whether it's being called with any arguments or not.

Comment 7

[Robert Sayre]

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.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: If we want no diff between JIT/interpreter, I guess we need this then

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.

Comment 9

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/de45be487415 for comment 8.

Comment 10

[Jeff Walden [:Waldo]]

Comment 9 got backed out, but http://hg.mozilla.org/tracemonkey/rev/4a65cc2a2872 relanded it.

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4a65cc2a2872

Comment 12

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/341f1d8f2d12

Comment 13

[Bob Clary [:bc] (inactive)]

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.

Comment 14

[Jeff Walden [:Waldo]]

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.

Comment 15

[Bob Clary [:bc] (inactive)]

test ecma_3/Object/8.6.2.6-002.js covers Bug 476624, not this bug.