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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: HE Shi-Jun, Assigned: Brian Crowder)

Tracking

({verified1.8.1.12})

Trunk
x86
Windows XP
verified1.8.1.12
Points:
---
Bug Flags:
blocking1.8.1.12 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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

Updated

10 years ago
Duplicate of this bug: 366864
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
Assignee: general → crowder
(Assignee)

Comment 5

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

Comment 6

10 years ago
Nevermind, looks like XML at least needs this defaultValue call
(Assignee)

Comment 7

10 years ago
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.
Attachment #251667 - Flags: review?(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
(Assignee)

Comment 9

10 years ago
Created attachment 251672 [details] [diff] [review]
v2
Attachment #251667 - Attachment is obsolete: true
Attachment #251672 - Flags: review?(brendan)
Attachment #251667 - Flags: review?(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
Attachment #251672 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

10 years ago
The general bug is bug 367199
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

10 years ago
Landed on trunk as jsfun.c 3.174

Comment 13

10 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-314874.js,v  <--  regress-314874.js
initial revision: 1.1
Flags: in-testsuite+

Comment 14

10 years ago
verified fixed linux, windows, mac* shell 20070406
Status: RESOLVED → VERIFIED

Updated

10 years ago
Flags: blocking1.8.1.12?
I'll let jwalden make the case, but in brief, this fix is wanted for Google Caja.

/be

Updated

10 years ago
Duplicate of this bug: 406337
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

9 years ago
(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?
Flags: blocking1.8.1.12? → blocking1.8.1.12+
(Assignee)

Comment 19

9 years ago
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
Keywords: fixed1.8.1.12

Comment 20

9 years ago
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
Keywords: fixed1.8.1.12 → verified1.8.1.12
You need to log in before you can comment on or make changes to this bug.