Last Comment Bug 314874 - Function.call/apply pass thisArg.valueOf() as the this value when thisArg is not a primitive value
: Function.call/apply pass thisArg.valueOf() as the this value when thisArg is ...
Status: VERIFIED FIXED
: verified1.8.1.12
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Brian Crowder
:
:
Mentors:
http://purl.oclc.org/NET/JavaScript/T...
: 366864 406337 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-02 19:53 PST by HE Shi-Jun
Modified: 2008-01-13 09:40 PST (History)
10 users (show)
dveditz: blocking1.8.1.12+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
objects are objects, don't need to valueOf them for call/apply (1.67 KB, patch)
2007-01-16 11:50 PST, Brian Crowder
no flags Details | Diff | Splinter Review
v2 (1.76 KB, patch)
2007-01-16 12:29 PST, Brian Crowder
brendan: review+
Details | Diff | Splinter Review

Description HE Shi-Jun 2005-11-02 19:53:05 PST
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] 2005-11-02 20:11:39 PST
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] 2005-11-02 20:17:24 PST
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 3 Aiko 2007-01-16 05:09:55 PST
*** Bug 366864 has been marked as a duplicate of this bug. ***
Comment 4 Brendan Eich [:brendan] 2007-01-16 11:14:44 PST
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 2007-01-16 11:24:45 PST
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 2007-01-16 11:28:03 PST
Nevermind, looks like XML at least needs this defaultValue call
Comment 7 Brian Crowder 2007-01-16 11:50:41 PST
Created attachment 251667 [details] [diff] [review]
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] 2007-01-16 12:26:08 PST
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 2007-01-16 12:29:26 PST
Created attachment 251672 [details] [diff] [review]
v2
Comment 10 Brendan Eich [:brendan] 2007-01-16 12:38:17 PST
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 2007-01-16 17:33:38 PST
The general bug is bug 367199
Comment 12 Brian Crowder 2007-01-16 17:36:06 PST
Landed on trunk as jsfun.c 3.174
Comment 13 Bob Clary [:bc:] 2007-04-03 21:09:43 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-314874.js,v  <--  regress-314874.js
initial revision: 1.1
Comment 14 Bob Clary [:bc:] 2007-04-06 10:38:43 PDT
verified fixed linux, windows, mac* shell 20070406
Comment 15 Brendan Eich [:brendan] 2007-12-02 14:12:08 PST
I'll let jwalden make the case, but in brief, this fix is wanted for Google Caja.

/be
Comment 16 John Resig 2007-12-02 17:38:36 PST
*** Bug 406337 has been marked as a duplicate of this bug. ***
Comment 17 Daniel Veditz [:dveditz] 2008-01-07 16:02:33 PST
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:] 2008-01-07 19:06:45 PST
(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 2008-01-11 10:26:48 PST
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:] 2008-01-13 09:40:24 PST
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

Note You need to log in before you can comment on or make changes to this bug.