Bug 18746 – SQUIRRELFISH: indirect eval used when direct eval should be used

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

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

This is also likely the cause of the failures of fast/js/kde/eval and fast/js/kde/scope.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Maciej Stachowiak]

Comment on attachment 20839 [details]
Revised proposed patch

r=me

[Cameron Zwarich (cpst)]

I didn't notice before, but this patch also fixes fast/js/for-in-exeception, for a total of 6 fixes.