Closed Bug 1440468 Opened 3 years ago Closed 2 years ago

Proxied functions can't be passed to Function.prototype.toString.call()

Categories

(Core :: JavaScript Engine, defect, P3)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: john.david.dalton, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

Attached image safari.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.167 Safari/537.36

Steps to reproduce:

var a = function() {}
var p = new Proxy(a, {})

Function.prototype.toString.call(p)
// TypeError: Function.prototype.toString called on incompatible object


typeof p // "function"
Object.prototype.toString.call(p) // [object Function]


With at least Safari passing it to Function.prototype.toString.call will work.
Related to https://bugs.chromium.org/p/v8/issues/detail?id=7484


Actual results:

It throws an error.


Expected results:

It should not throw.
Status: UNCONFIRMED → NEW
Component: Untriaged → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
I hadn't heard about this and I am sure I agree that this change is a good idea. I am going to look at the proposal.
Eewwwwwwww
Priority: -- → P3
Blocks: test262
Duplicate of this bug: 1487046
Assignee: nobody → evilpies
Comment on attachment 9006002 [details] [diff] [review]
Proxied functions can't be passed to Function.prototype.toString.call(). r?

I changed the error message, because "x is not a function" doesn't really make sense while allowing all callables.
Attachment #9006002 - Flags: review?(andrebargull)
evilpie++
Comment on attachment 9006002 [details] [diff] [review]
Proxied functions can't be passed to Function.prototype.toString.call(). r?

Review of attachment 9006002 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

And good idea to adjust the error message.
Attachment #9006002 - Flags: review?(andrebargull) → review+
By way of the `Function#toString` revisions `Function.prototype.toString.call` does allow proxied functions. This is now part of test262 and has been fixed in Chakra, V8, and JS Core, leaving FF as the only one not supporting this.
Attachment #9006103 - Flags: review?(ystartsev)
Comment on attachment 9006103 [details] [diff] [review]
Fix devtools test

Looks good to me
Attachment #9006103 - Flags: review?(ystartsev) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3528822f9b00
Proxied functions can't be passed to Function.prototype.toString.call(). r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/02675ca26d6c
Fix devtools test for new Function.prototype.toString behavior. r=yulia
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/3528822f9b00
https://hg.mozilla.org/mozilla-central/rev/02675ca26d6c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Target Milestone: mozilla64 → mozilla63
Target Milestone: mozilla63 → mozilla64
Note to docs team:

I have added a note covering this to the Fx 64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#JavaScript 

This probably needs describing in a sensible place in the docs, and maybe adding to BCD somehow too?
Duplicate of this bug: 1508968
It looks like this is part of https://github.com/tc39/Function-prototype-toString-revision and I've documented that when it got implemented initially: https://bugzilla.mozilla.org/show_bug.cgi?id=1317400#c52

The compat table (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/toString#Browser_compatibility) says Firefox 54, because that's when bug 1317400 landed. Should we update the table to say Firefox 64 and mark 54-63 as partial support for "String revision" then?

Should this be mentioned somewhere in the Proxy docs? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

Not sure if this is really a significant change that actually requires more detailed docs. Please let me know.
Flags: needinfo?(evilpies)
(In reply to Florian Scholz [:fscholz] (MDN) from comment #17)
> It looks like this is part of
> https://github.com/tc39/Function-prototype-toString-revision and I've
> documented that when it got implemented initially:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1317400#c52
> 
Great
> The compat table
> (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Function/toString#Browser_compatibility) says Firefox 54,
> because that's when bug 1317400 landed. Should we update the table to say
> Firefox 64 and mark 54-63 as partial support for "String revision" then?
> 
Sounds fine to me, or maybe just split this feature out?
> Should this be mentioned somewhere in the Proxy docs?
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Proxy
> 
I don't think we need to mention this on the Proxy page, but https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/toString#Description needs to be updated about not throwing for Proxy.
> Not sure if this is really a significant change that actually requires more
> detailed docs. Please let me know.
Flags: needinfo?(evilpies)
You need to log in before you can comment on or make changes to this bug.