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.
Created attachment 26821 [details] small eval test suite
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.
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.
Created attachment 27280 [details] Patch with fix and test suite. Following up with the fix for Chris Brichfort.
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
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.
Comment on attachment 27348 [details] Another fix and test suite r=me
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.