Bug 23400 – eval can return the incorrect result if an exception is thrown and caught

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

[Chris Brichford]

In the latest nightly ( r40000 ) on the mac, JSCore's eval will return the incorrect value for the following code:

eval("1; try { 2; throw \"\"; } catch (e){}");

JSCore's eval will return an empty string instead of 1.  Attached is a small test suite that tests various forms of eval strings.  All of the tests in the suite pass in FireFox 3 and IE 7.

[Chris Brichford]

Created attachment 26821 [details]
small eval test suite

[Chris Brichford]

I ran into this in WebKit as of r34190.  In that version of WebKit I was able to fix this problem, by changing ExprStatementNode::emitCode to be:

RegisterID* ExprStatementNode::emitCode(CodeGenerator& generator, RegisterID* dst)
{
    ASSERT(m_expr);
    RefPtr<RegisterID> value = generator.emitNode(m_expr.get());
    return dst ? generator.moveToDestinationIfNeeded(dst, value.get()) : value.get();
}

This is a somewhat crude fix, but hopefully safe fix.  It has the unfortunate side effect of causing every statement in an eval to put its result in a temporary and then move it to dst.  Code in functions in an eval is not affected, because dst for those statements is 0.

I think the correct fix may be to audit all uses of dst in Nodes.cpp to make sure dst is never used to store a temporary value unless all subsequent instruction emitted for the node can not throw an exception.  That seems very fragile and it the perf improvement to eval calls may not be worth the extra maintenance cost.

[Alexey Proskuryakov]

Confirmed with r39961.

Just to clarify, the result in shipping Safari/WebKit does not match expectations either (1 vs. "" vs. 2). The attached test has several examples where ToT behavior is different, but no changes from correct to incorrect.

[Horia Olaru]

Created attachment 27280 [details]
Patch with fix and test suite.

Following up with the fix for Chris Brichfort.

[Oliver Hunt]

Comment on attachment 27280 [details]
Patch with fix and test suite.

Have you perf tested this?  I'm concerned that ExprStatementNode::emitBytecode changes will result in an additional copy

[Horia Olaru]

Created attachment 27348 [details]
Another fix and test suite

After running the console sunspider tests the previous patch had results in the range of *1.006x as slow* compared to ToT.
Attaching a revised patch that improves performance.

[Oliver Hunt]

Comment on attachment 27348 [details]
Another fix and test suite

r=me

[Gavin Barraclough]

landed.

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/parser/Nodes.cpp
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/js/eval-throw-return-expected.txt
Adding         LayoutTests/fast/js/eval-throw-return.html
Adding         LayoutTests/fast/js/resources/eval-throw-return.js
Transmitting file data ......
Committed revision 40962.