Bug 18752 – SQUIRRELFISH: exceptions are not always handled by the vm

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

This has been fixed in r32756 except for the exception check on returns from native calls.

[Cameron Zwarich (cpst)]

Created attachment 20920 [details]
Proposed patch

[Maciej Stachowiak]

Since r32756 is implicated in a performance regression we should probably hold off on doing more of the same.

[Oliver Hunt]

(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 :-(

[Cameron Zwarich (cpst)]

(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.

[Oliver Hunt]

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.

[Oliver Hunt]

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

[Oliver Hunt]

Created attachment 20966 [details]
Better testcase

Now in html, and with tests for execution of the right hand side of the expressions

[Oliver Hunt]

Firefox fails in the *, -, /, and % cases

[Oliver Hunt]

Created attachment 20977 [details]
Test that can be run as JS or HTML

[Oliver Hunt]

SadEagle's sequencing test: http://websvn.kde.org/*checkout*/trunk/tests/khtmltests/regression/tests/js/sequencing.js?revision=782984&content-type=text%2Fplain

[Oliver Hunt]

Created attachment 21097 [details]
Yet more insane testing

[Oliver Hunt]

Fixed extraneous evaluation of RHS type conversions
	M	JavaScriptCore/API/JSCallbackObjectFunctions.h
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/kjs/object.cpp
Committed r33372

[Oliver Hunt]

Created attachment 21104 [details]
More coverage

[Oliver Hunt]

Created attachment 21105 [details]
Even more coverage

[Oliver Hunt]

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

[Oliver Hunt]

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