Syntax errors should be early errors.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

From the spec:

"16 Errors"

"An implementation must report most errors at the time the relevant ECMAScript language construct is evaluated. An early error is an error that can be detected and reported prior to the evaluation of any construct in the Program containing the error. An implementation must report early errors in a Program prior to the first evaluation of that Program. Early errors in eval code are reported at the time eval is called but prior to evaluation of any construct within the eval code. All errors that are not early errors are runtime errors."

"An implementation must treat any instance of the following kinds of errors as an early error:"
"• Any syntax error."

JavaScriptCore incorrectly raises various parse errors (return from eval, break/continue outside of switch/loop) as runtime errors.
Fixing this will match FireFox behavior.

[Gavin Barraclough]

Created attachment 78277 [details]
Work in progress

This patch may not be fully correct.
It makes most errors occur much earlier - which is good - but may still be leaving some slightly too late.
There should probably be no need to check for syntax errors at all during bytecode compilation - these should all be found by the parser.
In a number of cases (e.g. return) this can probably be trivially turned into an ASSERT.
In the case of breaks to labels we may need to move the checking that the target of the break is a valid breakTarget() up to the parser.

[Gavin Barraclough]

Created attachment 78362 [details]
Fixed patch (hopefully!)

New yarr files added with 'Yarr' prefix instead of 'Regex' to match bug #51872.

[Darin Adler]

Comment on attachment 78362 [details]
Fixed patch (hopefully!)

The EWS bot is unable to apply this patch.

[Gavin Barraclough]

Created attachment 78412 [details]
Updated patch

[WebKit Review Bot]

Attachment 78412 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/selection/select-crash-001.html', u'LayoutTests/editing/selection/select-crash-002.html', u'LayoutTests/fast/canvas/webgl/renderbuffer-initialization.html', u'LayoutTests/fast/forms/25153.html', u'LayoutTests/fast/forms/textfield-drag-into-disabled.html', u'LayoutTests/fast/js/exception-codegen-crash-expected.txt', u'LayoutTests/fast/js/exception-codegen-crash.html', u'LayoutTests/fast/js/kde/parse-expected.txt', u'LayoutTests/fast/js/kde/script-tests/parse.js', u'LayoutTests/fast/js/large-expressions-expected.txt', u'LayoutTests/fast/js/named-function-expression-expected.txt', u'LayoutTests/fast/js/parser-syntax-check-expected.txt', u'LayoutTests/fast/js/script-tests/large-expressions.js', u'LayoutTests/fast/js/script-tests/named-function-expression.js', u'LayoutTests/fast/js/script-tests/parser-syntax-check.js', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A11.1_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A11_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A12.1_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A12_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A1_T1-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A1_T2-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A1_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A1_T4-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A5_T1-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A5_T2-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A5_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A6-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A8_T1-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.7_The_continue_Statement/S12.7_A8_T2-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A1_T1-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A1_T2-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A1_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A1_T4-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A5_T1-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A5_T2-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A5_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A6-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A8_T1-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.8_The_break_Statement/S12.8_A8_T2-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T1-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T10-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T2-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T3-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T4-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T5-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T6-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T7-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T8-expected.txt', u'LayoutTests/fast/js/sputnik/Conformance/12_Statement/12.9_The_return_Statement/S12.9_A1_T9-expected.txt', u'LayoutTests/http/tests/security/isolatedWorld/events.html', u'LayoutTests/http/tests/security/isolatedWorld/userGestureEvents.html', u'LayoutTests/svg/custom/resources/use-instanceRoot-event-listeners.js', u'LayoutTests/svg/custom/rgbcolor-syntax.svg', u'LayoutTests/svg/custom/use-instanceRoot-modifications.svg', u'LayoutTests/svg/custom/use-property-changes-through-svg-dom.svg', u'LayoutTests/webarchive/adopt-attribute-styled-body-webarchive-expected.webarchive', u'LayoutTests/webarchive/resources/adopt-attribute-styled-body-iframe.html', u'Source/JavaScriptCore/Android.mk', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.am', u'Source/JavaScriptCore/JavaScriptCore.gypi', u'Source/JavaScriptCore/JavaScriptCore.pro', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/Opcode.h', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h', u'Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/parser/ASTBuilder.h', u'Source/JavaScriptCore/parser/JSParser.cpp', u'Source/JavaScriptCore/parser/JSParser.h', u'Source/JavaScriptCore/parser/Nodes.h', u'Source/JavaScriptCore/parser/Parser.cpp', u'Source/JavaScriptCore/parser/SyntaxChecker.h', u'Source/JavaScriptCore/runtime/ExceptionHelpers.cpp', u'Source/JavaScriptCore/runtime/ExceptionHelpers.h', u'Source/JavaScriptCore/runtime/Executable.cpp', u'Source/JavaScriptCore/runtime/RegExpConstructor.cpp', u'Source/JavaScriptCore/runtime/RegExpPrototype.cpp', u'Source/JavaScriptCore/yarr/RegexParser.h', u'Source/JavaScriptCore/yarr/RegexPattern.cpp', u'Source/JavaScriptCore/yarr/YarrSyntaxChecker.cpp', u'Source/JavaScriptCore/yarr/YarrSyntaxChecker.h']" exit_code: 1
Source/JavaScriptCore/yarr/YarrSyntaxChecker.h:37:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/yarr/YarrSyntaxChecker.h:37:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/parser/JSParser.cpp:139:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/parser/JSParser.cpp:152:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/yarr/YarrSyntaxChecker.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 78414 [details]
Style fixes

[Early Warning System Bot]

Attachment 78412 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7383093

[Oliver Hunt]

Comment on attachment 78414 [details]
Style fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=78414&action=review

You need to add tests for nested labels, so r- for that, but also i don't believe that you're right to make L1: while(1) { L1: while(1) {}} an error so i want to know what justifies that decision.

> Source/JavaScriptCore/parser/JSParser.cpp:1264
> +        failIfTrue(hasLabel(ident));

You have just made
L1: while(...) { L1:  while (...) { ... } }

A syntax error, which it never was before.  What portion of the spec says that this is an error?

[Gavin Barraclough]

(In reply to comment #8)
> (From update of attachment 78414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78414&action=review
> 
> You need to add tests for nested labels, so r- for that

Okay - will do - but I'm not sure what you want here -
    L1 : L2: statement?
Something like that?

> but also i don't believe that you're right to make L1: while(1) { L1: while(1) {}} an error so i want to know what justifies that decision.
> 
> > Source/JavaScriptCore/parser/JSParser.cpp:1264
> > +        failIfTrue(hasLabel(ident));
> 
> You have just made
> L1: while(...) { L1:  while (...) { ... } }
> 
> A syntax error, which it never was before.  What portion of the spec says that this is an error?

Sorry, I had a spec reference in a comment in an earlier form of the patch, will re add this.

Section 12.12 "An ECMAScript program is considered syntactically incorrect if it contains a LabelledStatement that is enclosed by a LabelledStatement with the same Identifier as label."

[Gavin Barraclough]

Oh did you mean you want a layout test for "L1: L1: Statement", to check it does throw an error?

[Oliver Hunt]

(In reply to comment #10)
> Oh did you mean you want a layout test for "L1: L1: Statement", to check it does throw an error?

Given your patch is meant to fix

L1:while(1) { L3: while(1) break L1; } // and continue ...

You should test variations along those lines for correct behaviour.

[Early Warning System Bot]

Attachment 78414 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7398093

[Gavin Barraclough]

(per discussion with Oliver sufficient test coverage exists.)

Fixed in r75408.

[Martin Robinson]

(In reply to comment #13)
> (per discussion with Oliver sufficient test coverage exists.)
> 
> Fixed in r75408.

fast/js/large-expressions.html is failing on some of the GTK+ bots with this diff:

 PASS eval(repeatedExpression("letterA", "+", 100)) is repeatedString("a", 100)
 PASS eval(repeatedExpression("letterA", "+", 1000)) is repeatedString("a", 1000)
 PASS eval(repeatedExpression("letterA", "+", 10000)) is repeatedString("a", 10000)
-PASS eval(repeatedExpression("letterA", "+", 100000)) threw exception Error: Out of memory.
+FAIL eval(repeatedExpression("letterA", "+", 100000)) should throw an exception. Was aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa (...etc...)

PASS successfullyParsed is true
TEST COMPLETE

[Gavin Barraclough]

We should probably change this test, to make it a little more tolerant.  The problem here is that GTK's results are a little too good. ;-)

This test is currently sensitive to stack depth, which can validly vary between platforms.  We should probably require the first couple of test cases to work (them throwing would probably indicate something is wrong), but for the latter two either throwing a SyntaxError or successfully completing could be considered a good result.


Something like this will probably work a little better:

function repeatedExpression(value, operator, count)
{
    var expression = value;
    for (var i = 1; i < count; ++i)
        expression += ' ' + operator + ' ' + value;
    return expression;
}

function testLargeExpression(count)
{
    try {
        return eval(repeatedExpression("letterA", "+", count)).length == count;
    } catch (e) {
        return count >= 10000 && String(e) == "SyntaxError: Expression too deep";
    }
}

shouldBeTrue('testLargeExpression(100)');
shouldBeTrue('testLargeExpression(1000)');
shouldBeTrue('testLargeExpression(10000)');
shouldBeTrue('testLargeExpression(100000)');