Bug 18746 - SQUIRRELFISH: indirect eval used when direct eval should be used
Summary: SQUIRRELFISH: indirect eval used when direct eval should be used
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 18631
  Show dependency treegraph
Reported: 2008-04-25 16:13 PDT by Cameron Zwarich (cpst)
Modified: 2008-04-27 22:12 PDT (History)
0 users

See Also:

Test case (127 bytes, text/html)
2008-04-25 19:43 PDT, Cameron Zwarich (cpst)
no flags Details
Proposed patch (2.03 KB, patch)
2008-04-26 14:34 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (2.05 KB, patch)
2008-04-26 14:58 PDT, Cameron Zwarich (cpst)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-04-25 16:13:07 PDT
This causes the layout test fast/js/read-modify-eval to fail. However, it only fails as a layout test, and the same code works fine if I try it out in testkjs.
Comment 1 Cameron Zwarich (cpst) 2008-04-25 18:49:32 PDT
I know this bug report is somewhat unclear, but I don't really know what is causing the problem. It throws a SyntaxError saying that x is undefined. Maciej suggests that it might be due to the global object being a window rather than a plain JSGlobalObject, and that it could be related to the split window change.
Comment 2 Cameron Zwarich (cpst) 2008-04-25 19:43:46 PDT
Created attachment 20827 [details]
Test case

Here is a test case showing the bug. It should print PASS, but instead prints FAIL. This only occurs in the browser, not in testkjs. The problem seems to be that eval is being passed an incorrect scope chain. This is also causing the failure of fast/js/kde/assignments.
Comment 3 Cameron Zwarich (cpst) 2008-04-25 19:52:23 PDT
Some quick debugging shows that while op_call_eval is being used, the following test for whether it is a direct eval fails:

if (base == exec->lexicalGlobalObject() && v == exec->lexicalGlobalObject()->evalFunction()) {

The first part of the conditional is false, the second part is true. This causes it to fall back to an indirect eval. The addresses of base and exec->lexicalGlobalObject() are 0x100f0000 and 0x100f0020 respectively.
Comment 4 Cameron Zwarich (cpst) 2008-04-25 20:02:04 PDT
This is also likely the cause of the failures of fast/js/kde/eval and fast/js/kde/scope.
Comment 5 Cameron Zwarich (cpst) 2008-04-25 23:27:42 PDT
Changing the direct eval check to

if (base == exec->globalThisValue() && v == exec->lexicalGlobalObject()->evalFunction()) {

fixes almost all of the eval tests. The only exceptions, which it changes from passing to failing, are tests of the form

(function() { var eval = window.eval; return eval("var y; "y" in window"); })()

Here, op_call_eval is emitted for the eval, and the new direct eval check erroneously returns that it is a direct eval. Since this is the same test used for direct eval on trunk with the split window patch, it seems the best way to fix this is to get the split window patch fully functional on SquirrelFish.
Comment 6 Cameron Zwarich (cpst) 2008-04-26 14:34:34 PDT
Created attachment 20835 [details]
Proposed patch

Here is a proposed patch. It fixes the 4 eval layout tests as well fast/js/encode-URI-test. Unfortunately, on my machine it is a 0.8% regression on SunSpider. I'll try doing the usual tricks, but maybe somebody can check it on their machine.
Comment 7 Cameron Zwarich (cpst) 2008-04-26 14:58:19 PDT
Created attachment 20839 [details]
Revised proposed patch

It seems I didn't have an up-to-date tree. This applies cleanly to the current tree, and it is now a wash on SunSpider on my machine.
Comment 8 Maciej Stachowiak 2008-04-26 15:04:51 PDT
Comment on attachment 20839 [details]
Revised proposed patch

Comment 9 Cameron Zwarich (cpst) 2008-04-26 23:18:40 PDT
I didn't notice before, but this patch also fixes fast/js/for-in-exeception, for a total of 6 fixes.