Closed
Bug 314874
Opened 18 years ago
Closed 17 years ago
Function.call/apply pass thisArg.valueOf() as the this value when thisArg is not a primitive value
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: hax.sfo, Assigned: crowderbt)
References
()
Details
(Keywords: verified1.8.1.12)
Attachments
(1 file, 1 obsolete file)
1.76 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
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
Comment 2•18 years ago
|
||
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•17 years ago
|
||
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•17 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•17 years ago
|
||
Nevermind, looks like XML at least needs this defaultValue call
Assignee | ||
Comment 7•17 years ago
|
||
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 8•17 years ago
|
||
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•17 years ago
|
||
Attachment #251667 -
Attachment is obsolete: true
Attachment #251672 -
Flags: review?(brendan)
Attachment #251667 -
Flags: review?(brendan)
Comment 10•17 years ago
|
||
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•17 years ago
|
||
The general bug is bug 367199
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•17 years ago
|
||
Landed on trunk as jsfun.c 3.174
Comment 13•17 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•17 years ago
|
||
verified fixed linux, windows, mac* shell 20070406
Status: RESOLVED → VERIFIED
Updated•16 years ago
|
Flags: blocking1.8.1.12?
Comment 15•16 years ago
|
||
I'll let jwalden make the case, but in brief, this fix is wanted for Google Caja. /be
Comment 17•16 years ago
|
||
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•16 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?
Updated•16 years ago
|
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Assignee | ||
Comment 19•16 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•16 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.
Description
•