Bug 18752 - SQUIRRELFISH: exceptions are not always handled by the vm
Summary: SQUIRRELFISH: exceptions are not always handled by the vm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-26 00:02 PDT by Cameron Zwarich (cpst)
Modified: 2008-05-18 17:36 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (1.19 KB, patch)
2008-05-02 00:29 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Test case for various and exciting combinations of throwing and arithmetic (2.59 KB, text/plain)
2008-05-04 23:33 PDT, Oliver Hunt
no flags Details
Better testcase (3.62 KB, text/html)
2008-05-05 00:13 PDT, Oliver Hunt
no flags Details
Test that can be run as JS or HTML (3.63 KB, text/html)
2008-05-05 19:44 PDT, Oliver Hunt
no flags Details
Yet more insane testing (5.95 KB, text/html)
2008-05-12 20:06 PDT, Oliver Hunt
no flags Details
More coverage (9.71 KB, text/html)
2008-05-13 01:23 PDT, Oliver Hunt
no flags Details
Even more coverage (10.04 KB, text/html)
2008-05-13 01:33 PDT, Oliver Hunt
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-04-26 00:02:19 PDT
Here is an example:

var x = "PASS";
try { x = (new Number(1)).toString(0); } catch (e) { }
print(x);

On trunk, it prints PASS, but on SquirrelFish it prints an exception. This is because the VM_CHECK_EXCEPTION(); statements in many opcodes in Machine.cpp are after the writes to registers, which may store local variables. It shouldn't be too hard to move all of the checks out, and hopefully it won't be another performance regression due to compiler insanity.
Comment 1 Cameron Zwarich (cpst) 2008-05-02 00:21:25 PDT
This has been fixed in r32756 except for the exception check on returns from native calls.
Comment 2 Cameron Zwarich (cpst) 2008-05-02 00:29:08 PDT
Created attachment 20920 [details]
Proposed patch
Comment 3 Maciej Stachowiak 2008-05-02 04:05:33 PDT
Since r32756 is implicated in a performance regression we should probably hold off on doing more of the same.
Comment 4 Oliver Hunt 2008-05-02 09:43:45 PDT
(In reply to comment #1)
> This has been fixed in r32756 except for the exception check on returns from
> native calls.
> 

That rev only fixed op_resolve, given there are multiple resolves, and basically every other op can throw this is not anywhere near fixed :-(
Comment 5 Cameron Zwarich (cpst) 2008-05-02 14:50:41 PDT
(In reply to comment #4)
> (In reply to comment #1)
> > This has been fixed in r32756 except for the exception check on returns from
> > native calls.
> 
> That rev only fixed op_resolve, given there are multiple resolves, and
> basically every other op can throw this is not anywhere near fixed :-(

Oh yeah, I was only checking for VM_CHECK_EXCEPTION() in Machine::privateExecute(), but there are other exception checks in external functions.
Comment 6 Oliver Hunt 2008-05-02 14:56:41 PDT
It also effects most of the math operators, etc, etc

The problem is a number of the places where we do bogus assignments are also in opcodes where we don't do sufficient/any exception checks. (all the numeric operations for instance.
Comment 7 Oliver Hunt 2008-05-04 23:33:44 PDT
Created attachment 20965 [details]
Test case for various and exciting combinations of throwing and arithmetic

Here is a test case that triggers (and tests many exciting issues
Comment 8 Oliver Hunt 2008-05-05 00:13:35 PDT
Created attachment 20966 [details]
Better testcase

Now in html, and with tests for execution of the right hand side of the expressions
Comment 9 Oliver Hunt 2008-05-05 00:41:35 PDT
Firefox fails in the *, -, /, and % cases
Comment 10 Oliver Hunt 2008-05-05 19:44:10 PDT
Created attachment 20977 [details]
Test that can be run as JS or HTML
Comment 12 Oliver Hunt 2008-05-12 20:06:55 PDT
Created attachment 21097 [details]
Yet more insane testing
Comment 13 Oliver Hunt 2008-05-13 00:12:02 PDT
Fixed extraneous evaluation of RHS type conversions
	M	JavaScriptCore/API/JSCallbackObjectFunctions.h
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/kjs/object.cpp
Committed r33372
Comment 14 Oliver Hunt 2008-05-13 01:23:05 PDT
Created attachment 21104 [details]
More coverage
Comment 15 Oliver Hunt 2008-05-13 01:33:51 PDT
Created attachment 21105 [details]
Even more coverage
Comment 16 Oliver Hunt 2008-05-13 14:52:08 PDT
More fixes (all toNumber, and numerous other places now have correct checks)
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/VM/Machine.cpp
	M	JavaScriptCore/VM/Opcode.h
	M	JavaScriptCore/kjs/value.h
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/js/exception-sequencing-expected.txt
	A	LayoutTests/fast/js/exception-sequencing.html
Committed r33393
Comment 17 Oliver Hunt 2008-05-18 17:36:57 PDT
Committing to http://svn.webkit.org/repository/webkit/branches/squirrelfish ...
        M       JavaScriptCore/ChangeLog
        M       JavaScriptCore/VM/Machine.cpp
        M       LayoutTests/ChangeLog
        A       LayoutTests/fast/js/resources/tostring-exception-in-property-access.js
        A       LayoutTests/fast/js/tostring-exception-in-property-access-expected.txt
        A       LayoutTests/fast/js/tostring-exception-in-property-access.html
Committed r33566