WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
7831
.apply/call does not set this for eval
https://bugs.webkit.org/show_bug.cgi?id=7831
Summary
.apply/call does not set this for eval
Francisco Tolmasky
Reported
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).
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
View All
Add attachment
proposed patch, testcase, etc.
Francisco Tolmasky
Comment 1
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.
Francisco Tolmasky
Comment 2
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.
Maciej Stachowiak
Comment 3
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.
Geoffrey Garen
Comment 4
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().
Geoffrey Garen
Comment 5
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!)
Francisco Tolmasky
Comment 6
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 .
Darin Adler
Comment 7
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?
David Kilzer (:ddkilzer)
Comment 8
2008-07-20 06:49:15 PDT
This appears to be fixed (the test case no longer alerts) with WebKit nightly build
r35249
.
David Kilzer (:ddkilzer)
Comment 9
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.
David Kilzer (:ddkilzer)
Comment 10
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
David Kilzer (:ddkilzer)
Comment 11
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.
David Kilzer (:ddkilzer)
Comment 12
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.
David Kilzer (:ddkilzer)
Comment 13
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
David Kilzer (:ddkilzer)
Comment 14
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.
Oliver Hunt
Comment 15
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug