Bug 23400 - eval can return the incorrect result if an exception is thrown and caught
Summary: eval can return the incorrect result if an exception is thrown and caught
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-16 19:24 PST by Chris Brichford
Modified: 2009-02-12 20:11 PST (History)
3 users (show)

See Also:


Attachments
small eval test suite (2.66 KB, text/html)
2009-01-16 19:25 PST, Chris Brichford
no flags Details
Patch with fix and test suite. (7.00 KB, patch)
2009-02-03 08:18 PST, Horia Olaru
no flags Details | Formatted Diff | Diff
Another fix and test suite (6.85 KB, patch)
2009-02-05 07:36 PST, Horia Olaru
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Brichford 2009-01-16 19:24:17 PST
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.
Comment 1 Chris Brichford 2009-01-16 19:25:19 PST
Created attachment 26821 [details]
small eval test suite
Comment 2 Chris Brichford 2009-01-16 19:32:08 PST
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.
Comment 3 Alexey Proskuryakov 2009-01-17 04:14:47 PST
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.
Comment 4 Horia Olaru 2009-02-03 08:18:40 PST
Created attachment 27280 [details]
Patch with fix and test suite.

Following up with the fix for Chris Brichfort.
Comment 5 Oliver Hunt 2009-02-03 08:27:28 PST
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
Comment 6 Horia Olaru 2009-02-05 07:36:10 PST
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 7 Oliver Hunt 2009-02-12 15:21:03 PST
Comment on attachment 27348 [details]
Another fix and test suite

r=me
Comment 8 Gavin Barraclough 2009-02-12 20:11:03 PST
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.