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).
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.
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 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.
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().
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!)
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 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?
This appears to be fixed (the test case no longer alerts) with WebKit nightly build r35249.
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.
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
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.
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.
(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
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.
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.