987243 - Don't use .call(...) in self-hosted code

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

Using .call(...) in self-hosted code is either a bug, because .call can be overwritten, or it's not optimal for perf, because there's a callprop/etc. of overhead to eat.  (At least, assuming the spec doesn't directly call for "get the 'call' property and then call it with callee and arguments...", which it never has yet.)  Any use should instead be callFunction.

I have no idea why I can't create a testcase that fails because of this, but I can't.  I tried stepping through in a debugger, and std_Map_has and such were *not* the same as the ones in the actual script.  (Maybe the ones in the SHG?)  Apparently nobody understands this wrinkle of self-hosting code, because the people I pointed it out to either thought it was a bug, or agreed with me that it was a bug when I explained why I thought it was.  Wonderful.  Let's just fix this and not hugely overthink things, I guess.

Comment 1

[Till Schneidereit [:till]]

Comment on attachment 8395778 [details] [diff] [review]
Patch

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

r=me

Comment 2

[Till Schneidereit [:till]]

Also, as a note to evilpie and sankha, callFunction and why it exists is described here:
https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/self-hosting

Implementers and reviewers working on self-hosting code should probably study that page carefully. If in doubt, r? me.

Comment 3

[Jeff Walden [:Waldo]]

With some extra tests for issues as demonstrated by till -- need to overwrite Function.prototype.call to expose issues, overwriting .call on std_*_* isn't a workable demo:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d272a61907

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5d272a61907