Bug 7831 – .apply/call does not set this for eval

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Francisco Tolmasky]

Since eval simply ignores the this argument and always opts for the calling context's this or the global object, using eval.apply(something, args) does not work.  ECMA is somewhat unclear about this (10.2 says to use the calling context's this object), but I think it's pretty clear that if you explicitly ask to use another this then you should, given that that is basically the point of apply/call.  Firefox handles this the same way Safari currently does (incorrectly in my opinion).

[Francisco Tolmasky]

Created attachment 7132 [details]
test case

when this.x=9 is done, window.x is set instead of a.x, which is why "undfined" is then alerted.  I believe "9" should be alerted.

[Francisco Tolmasky]

Created attachment 7133 [details]
Makes eval.apply/call pay attention to the this argument.

What this does is make callAsFunction for eval check whether there is in fact a thisObj, which only takes place I believe if its called through apply or call.  In this case, if its not undefined or null, it uses it.  ContextImp now also checks for a this argument in eval if which again should only take place if we called it initially through apply or call.

[Maciej Stachowiak]

Comment on attachment 7133 [details]
Makes eval.apply/call pay attention to the this argument.

The substance of the change looks good. I have some style concerns:

1) I don't think you need comments explaining what the code used to do or why it was wrong. That can go in the ChangeLog.

2) This if statement shouldn't be all on one line.

+		if(!thisObj||thisObj->isUndefinedOrNull()) thisObj= static_cast<JSObject *>(exec->context().thisValue());

Can thisObj ever actually truly be a null pointer as opposed to JS null?

3) Needs a ChangeLog entry.

4) Needs a test case - you can look in LayoutTests/fast/js/resources/*.js, make a JS file like that using shouldBe and then use make-js-test-wrappers to make the HTML wrapper.

r- for ChangeLog and test case, but the substance of the change looks correct.

[Geoffrey Garen]

No, thisObj can never be a NULL pointer. It's important to leave NULL checks out of code that works with JSValues, because they might confuse folks into thinking that it's OK to return NULL rather than jsNull().

[Geoffrey Garen]

Also, please execute run-javascriptcore-tests to make sure this patch doesn't cause any regressions in the mozilla JS tests. (And maybe it will fix a few!)

[Francisco Tolmasky]

Created attachment 7146 [details]
Modified Patch

I ended up having to make a significantly different patch for this.  The old one, while working, caused a regression in Expressions/1.11.1.js or so.  The problem is that callAsFunction always gets sent a thisObj, so its not enough to check whether thisObj isn't null or undefined to see whether we are actually being called from an apply or call.  So the solution I came up with was to trick it by creating an inbetween ExecState that contains the correct thisObj.  I wanted to float this by you guys as whether it was an acceptible solution or not before writing a changelog/tests for it, since it does seem a bit hackish.  Also, this new patch fixes js1_6/Array/regress-304828.js .

[Darin Adler]

Comment on attachment 7146 [details]
Modified Patch

Thanks for taking this on!

There are many minor things wrong with this patch.

1) no tests -- patches that fix bugs require tests that demonstrate the bug is fixed (although I suppose the fact that this fixes on of the Mozilla tests might be an OK substitute)
2) no change log entry
3) commented-out code left in internal.cpp, should not be
4) patch that changes only whitespace in function.cpp
5) tabs in the patch (we use spaces for indenting)
6) no space between "if" and the "(" following it in the code in function_object.cpp
7) no spaces around the "==" operator
8) "ContextImp *cpy= " should be "ContextImp* cpy = ".
9) We don't declare multiple variables with a single declaration as is done with "cpy" and "ctx".
10) local variable named "cpy" -- "copy" would be better
11) local variable named "ctx" -- name that is either a word or just a single letter would be better
12) no spaces after commas in some function calls

However, the main issue here is almost certainly performance. Adding an inherits check to every function call is probably not a good idea. And as to the reason that's needed, I don't see what information FunctionProtoFunc::callAsFunction has that GlobalFuncImp::callAsFunction will not have.

Won't the internal.cpp change fix the bug without any change to function_object.cpp at all? If not, what's the high level description of what the function_object.cpp change does?

[David Kilzer (:ddkilzer)]

This appears to be fixed (the test case no longer alerts) with WebKit nightly build r35249.

[David Kilzer (:ddkilzer)]

Oops...since nothing is alerted on the nightly WebKit (expected is that "9" is alerted per Comment #1), this is a change in behavior that should probably be investigated.  Reopening.

[David Kilzer (:ddkilzer)]

Firefox 2.0.0.x does not show an alert, either.  Firebug lists the error as:

function eval must be called directly, and not by way of a function of another name

[David Kilzer (:ddkilzer)]

Running a bisect of nightly builds to find when the behavior changed.  Interestingly, nightly builds r27811-r28007 all crashed with a null pointer running the test case.

[David Kilzer (:ddkilzer)]

The bisect-builds script reports:

Fails: r30868  Works: r30885

Where "Fails" means an alert was displayed with "undefined" in the text of the alert, and "Works" means no alert was displayed.

Again, the expected result per Comment #1 is that "9" is displayed.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> The bisect-builds script reports:
> 
> Fails: r30868  Works: r30885

The only interesting commit in this range is:

http://trac.webkit.org/changeset/30871

[David Kilzer (:ddkilzer)]

Created attachment 22388 [details]
test case for exception

This test case tests that an exception is thrown in the expression.  This seems to jibe with the changes in r30871.

I'd suggest closing this bug in light of the above information.

[Oliver Hunt]

This behaviour is "correct".  Clearly firefox does the eval aliasing check before it does the target object checking which results in a different failure.  However this doesn't change the fact that eval can not be called in this manner.