314874 - Function.call/apply pass thisArg.valueOf() as the this value when thisArg is not a primitive value

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[HE Shi-Jun]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

Function.call/apply pass thisArg.valueOf() as the this value when thisArg.valueOf() returns a non-primitive value.

So if thisArg.valueOf() return an Object, f.call(thisArg) will be inconsistent with:
thisArg.f = f; thisArg.f(); delete thisArg.f

The behaivor here is complex and strange that only if thisArg.valueOf() returns primitive value, thisArg will be passed untouched, or it will be replaced by the result of thisArg.valueOf()

I have read the ECMA-262 3rd Edition, it is said:
If thisArg is null or undefined, the called function is passed the global object as the this value.
Otherwise, the called function is passed ToObject(thisArg) as the this value.

And as spec, ToObject()'s return result is the input argument (no conversion).
There should be no call of valueOf().

BTW, MSIE's behavior is correct.

Reproducible: Always

Steps to Reproduce:
See http://purl.oclc.org/NET/JavaScript/TestApplyCall.html

Comment 1

[Brendan Eich [:brendan]]

This is an extension to ECMA-262.  It actually predates ECMA standardizing JS (it goes back to Netscape 2 in 1995).  When Netscape joined ECMA to standardize JS as ECMA-262, we didn't convince other companies involved in the process to agree on making this extension part of the standard.  But we have had it in Netscape's and Mozilla's JS engines since 1995.  

After all these years, the obvious fix of removing the valueOf call for object to object conversion may break something we care about.  The only way to tell is to try it during 1.9alpha and see what breaks.

Compatibility is hard.  Confirming bug, leaving it for someone to take.

/be

Comment 2

[Brendan Eich [:brendan]]

Of course Function.prototype.{apply,call} are new in ECMA-262 Edition 3, not in Edition 1, so the general fix I was describing in comment 1, the fix that would remove valueOf calling on all object-to-object conversions, is a broader fix than this bug alone wants.  We could fix just fun_apply and fun_call in jsfun.c.  See

3.2          (fur      24-Apr-98):     if (!js_ValueToObject(cx, argv[0], &obj))

Note this is still over seven years old, so may break someone to change.  I am in favor of fixing the general bug, if we can afford to, by removing all valueOf   calling for object-to-object conversions.

/be

Comment 4

[Brendan Eich [:brendan]]

Brian, can I interest you in fixing the general bug described in comment 1? Or at least this specific case in fun_apply and fun_call? Thanks.

/be

Comment 5

[Brian Crowder]

Is making js_ValueToObject just return "v" in the JSVAL_IS_OBJECT case an acceptable fix, or too broad? (or just plain wrong?)

Comment 6

[Brian Crowder]

Nevermind, looks like XML at least needs this defaultValue call

Comment 7

[Brian Crowder]

Attachment: objects are objects, don't need to valueOf them for call/apply

Something like this?  Seems to work as I expect (and as the original poster suggests) in my JS shell.  I can't get to his original testcase, though: the internet is like a big truck today.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 251667 [details] [diff] [review]
objects are objects, don't need to valueOf them for call/apply

Mentioned JSVAL_IS_OBJECT being the wrong test over IRC, will r+ new patch.

/be

Comment 9

[Brian Crowder]

Attachment: v2

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 251672 [details] [diff] [review]
v2

r=me, thanks.  The general bug deserves to be spun out (avoid a dup if there's one on file already).

/be

Comment 11

[Brian Crowder]

The general bug is bug 367199

Comment 12

[Brian Crowder]

Landed on trunk as jsfun.c 3.174

Comment 13

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-314874.js,v  <--  regress-314874.js
initial revision: 1.1

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed linux, windows, mac* shell 20070406

Comment 15

[Brendan Eich [:brendan]]

I'll let jwalden make the case, but in brief, this fix is wanted for Google Caja.

/be

Comment 17

[Daniel Veditz [:dveditz]]

testcase seems to be unreachable, can someone attach one to the bug for QA branch verification? I don't think we can take this on the branch without that.

Comment 18

[Bob Clary [:bc] (inactive)]

(In reply to comment #17)
> testcase seems to be unreachable, can someone attach one to the bug for QA
> branch verification? I don't think we can take this on the branch without that.
> 

is js/tests/js1_5/extensions/regress-314874.js insufficient?

Comment 19

[Brian Crowder]

The patch "v2" applied cleanly on the MOZILLA_1_8_BRANCH with only 1 line offsets on both hunks:

jsfun.c: 3.117.2.32

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1 linux/mac/windows

/cvsroot/mozilla/js/tests/public-failures.txt,v  <--  public-failures.txt
new revision: 1.27; previous revision: 1.26