Bug 7831 - .apply/call does not set this for eval
Summary: .apply/call does not set this for eval
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2006-03-17 10:41 PST by Francisco Tolmasky
Modified: 2008-07-20 13:30 PDT (History)
5 users (show)

See Also:


Attachments
test case (70 bytes, text/html)
2006-03-17 10:42 PST, Francisco Tolmasky
no flags Details
Makes eval.apply/call pay attention to the this argument. (2.21 KB, patch)
2006-03-17 10:48 PST, Francisco Tolmasky
mjs: review-
Details | Formatted Diff | Diff
Modified Patch (3.39 KB, patch)
2006-03-18 01:25 PST, Francisco Tolmasky
darin: review-
Details | Formatted Diff | Diff
test case for exception (125 bytes, text/html)
2008-07-20 07:52 PDT, David Kilzer (:ddkilzer)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Francisco Tolmasky 2006-03-17 10:41:03 PST
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).
Comment 1 Francisco Tolmasky 2006-03-17 10:42:36 PST
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.
Comment 2 Francisco Tolmasky 2006-03-17 10:48:46 PST
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.
Comment 3 Maciej Stachowiak 2006-03-17 11:48:09 PST
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.
Comment 4 Geoffrey Garen 2006-03-17 13:49:47 PST
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().
Comment 5 Geoffrey Garen 2006-03-17 13:51:58 PST
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!)
Comment 6 Francisco Tolmasky 2006-03-18 01:25:18 PST
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 .
Comment 7 Darin Adler 2006-03-19 16:30:49 PST
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?
Comment 8 David Kilzer (:ddkilzer) 2008-07-20 06:49:15 PDT
This appears to be fixed (the test case no longer alerts) with WebKit nightly build r35249.

Comment 9 David Kilzer (:ddkilzer) 2008-07-20 06:52:22 PDT
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.

Comment 10 David Kilzer (:ddkilzer) 2008-07-20 06:54:28 PDT
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

Comment 11 David Kilzer (:ddkilzer) 2008-07-20 07:20:33 PDT
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.

Comment 12 David Kilzer (:ddkilzer) 2008-07-20 07:34:24 PDT
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.

Comment 13 David Kilzer (:ddkilzer) 2008-07-20 07:48:07 PDT
(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

Comment 14 David Kilzer (:ddkilzer) 2008-07-20 07:52:59 PDT
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.
Comment 15 Oliver Hunt 2008-07-20 13:30:51 PDT
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.