Bug 62613 – No context for javascript parse errors.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Juan C. Montemayor]

For parse errors in javascript files, the Web Inspector prints out the message: "SyntaxError: Parse Error" which is not very useful.

What we want here is some more context for parse errors, of this form:
    SyntaxError: Unexpected token: 'TOKEN'

[Juan C. Montemayor]

Created attachment 97047 [details]
proposed patch

[Oliver Hunt]

Comment on attachment 97047 [details]
proposed patch

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

The logic for this patch looks fine, however you need to have a slightly more detailed changelog (basically saying what the change you're making is).

Normally you'd be expected to have a new testcase for this, but i suspect if you do run-webkit-tests you'll see a large number of failures due to the changed output.  Depending on what the changes are we may consider the new test outputs to be sufficient on their own, otherwise you'll need to write a new test specifically.

r- due to the test problem.

> Source/JavaScriptCore/ChangeLog:7
> +

You should have a more detailed description here.

[Geoffrey Garen]

Comment on attachment 97047 [details]
proposed patch

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

> Source/JavaScriptCore/parser/JSParser.cpp:156
> +        m_errorMessage = UString(String::format("Unexpected token '%c'", source[tokenStart()]).impl());

Is one character of context enough? What does this print when the unexpected token is "return" or "break" or "continue" or "if" or something like that? I think you may need the full token as a string.

e.g.:

function f() { return if; } // Should be: "Unexpected token: 'if'".

return; // Should be: "Unexpected token: 'return'".

break; // Should be: "Unexpected token: 'break'".

continue; // Should be: "Unexpected token: 'continue'".

a[0]]; // I think you have this one right: "Unexpected token: ']'".

[Oliver Hunt]

(In reply to comment #3)
> (From update of attachment 97047 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97047&action=review
> 
> > Source/JavaScriptCore/parser/JSParser.cpp:156
> > +        m_errorMessage = UString(String::format("Unexpected token '%c'", source[tokenStart()]).impl());
> 
> Is one character of context enough? What does this print when the unexpected token is "return" or "break" or "continue" or "if" or something like that? I think you may need the full token as a string.
> 
> e.g.:
> 
> function f() { return if; } // Should be: "Unexpected token: 'if'".
> 
> return; // Should be: "Unexpected token: 'return'".
> 
> break; // Should be: "Unexpected token: 'break'".
> 
> continue; // Should be: "Unexpected token: 'continue'".
> 
> a[0]]; // I think you have this one right: "Unexpected token: ']'".

We've discussed other bits in person;  JC is going to work on improving the error messages a bit from what the current patch does.

[Juan C. Montemayor]

Created attachment 97382 [details]
proposed patch

[Juan C. Montemayor]

Error messages now have more context, like  
        "Unexpected token: ]"
        "Expected token: while"        
        "Use of reserved word: super"
        "Unexpected number: 42"
        "Unexpected identifier: "
        "Unexpected string: "foobar""
        "Invalid token character sequence: \u4023"
        "Unexpected EOF"

The test 'fast/js/script-tests/parser-syntax-check.js' was fixed to accept the new parser messages.

The expected results for the fast/js/ and sputnik tests were updated to reflect the new changes.

[Oliver Hunt]

Comment on attachment 97382 [details]
proposed patch

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

The bulk of the patch is looking good, but still needs a bit of tidying in the logic and structure but not too much (see my comments below), but the error messages need a bit more work.

We should just file a bug on the lexer errors and work on that once we've got the parser errors sorted out and landed.

> Source/JavaScriptCore/parser/JSParser.cpp:135
> +    ALWAYS_INLINE String getToken() {

In JSC we use UString rather than String (yes i realise there both essentially RefPtr<StringImpl>, but while a foolish consistency may be the hobgoblin of a little mind, code consistency is awesome!)

> Source/JavaScriptCore/parser/JSParser.cpp:184
> +                break;

Incorrect indenting

> Source/JavaScriptCore/parser/JSParser.cpp:328
> +        case LastUntaggedToken: 
> +            ASSERT_NOT_REACHED();
> +            break;

I'd put this one at the end

> Source/JavaScriptCore/parser/JSParser.cpp:346
> +        case AUTOPLUSPLUS: 
> +            name = "auto ++"; 
> +            break;
> +        case AUTOMINUSMINUS: 
> +            name = "auto --"; 
> +            break;

These are internal concepts, you should just make the above PLUSPLUS logic also handle AUTOPLUSPLUS, eg.
case AUTOPLUSPLUS:
case PLUSPLUS:
   ...

(and the same for --)

> Source/JavaScriptCore/parser/JSParser.cpp:426
> +        return name;

Given we're just returning name, it would probably be better to replace the

   name = ...;
   break;

pattern with

    return ...;

And then put
    ASSERT_NOT_REACHED();
    return "internal error";

at the end.

> Source/JavaScriptCore/parser/JSParser.cpp:450
> +            errorMessage = "Invalid token character sequence: ";

I think "Unrecognised token: " but i'm notoriously bad with such things so we may want to ask for input from geoff (in future the lexer should provide a meaningful message)

> Source/JavaScriptCore/parser/JSParser.cpp:481
> +            m_errorMessage = UString(String::format("Expected token: %s", name).impl());

We may want this message to be:

Expected <token that we want> but got <token that we got> instead.

> LayoutTests/fast/js/basic-strict-mode-expected.txt:31
> +PASS (function eval(){'use strict';}) threw exception SyntaxError: Unexpected token: }.

looking at these i wonder if we want quotes around the symbol tokens?

> LayoutTests/fast/js/basic-strict-mode-expected.txt:79
> +PASS 'use strict'; return threw exception SyntaxError: Unexpected token: return.

Better error would be
"return is only inside of a function"

> LayoutTests/fast/js/basic-strict-mode-expected.txt:81
> +PASS 'use strict'; break threw exception SyntaxError: Unexpected token 'break'.
> +PASS (function(){'use strict'; break}) threw exception SyntaxError: Unexpected token 'break'.

Better error would be
"break is only valid inside a switch or loop statement"

> LayoutTests/fast/js/basic-strict-mode-expected.txt:83
> +PASS 'use strict'; continue threw exception SyntaxError: Unexpected token 'continue'.
> +PASS (function(){'use strict'; continue}) threw exception SyntaxError: Unexpected token 'continue'.

Better error would be
"continue is only valid inside a loop statement"

> LayoutTests/fast/js/basic-strict-mode-expected.txt:88
> +PASS (function(){'use strict'; for(;;)continue missingLabel}) threw exception SyntaxError: Unexpected identifier: missingLabel (in strict mode).

Better error here is:
"Label '<label name>' is not defined"

> LayoutTests/fast/js/basic-strict-mode-expected.txt:94
> +PASS 'use strict'; 007; threw exception SyntaxError: Invalid token character sequence: 007 (in strict mode).
> +PASS (function(){'use strict'; 007;}) threw exception SyntaxError: Invalid token character sequence: 007 (in strict mode).
> +PASS 'use strict'; '\007'; threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode).
> +PASS (function(){'use strict'; '\007';}) threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode).
> +PASS '\007'; 'use strict'; threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode).
> +PASS (function(){'\007'; 'use strict';}) threw exception SyntaxError: Invalid token character sequence: '\0 (in strict mode).

These will be fixed when we make the lexer errors better

> LayoutTests/fast/js/basic-strict-mode-expected.txt:100
> +PASS 'use strict'; delete aDeletableProperty; threw exception SyntaxError: Unexpected token: ;.
> +PASS (function(){'use strict'; delete aDeletableProperty;}) threw exception SyntaxError: Unexpected token: ;.
> +PASS 'use strict'; (function (){ delete someDeclaredGlobal;}) threw exception SyntaxError: Unexpected token: ;.
> +PASS (function(){'use strict'; (function (){ delete someDeclaredGlobal;})}) threw exception SyntaxError: Unexpected token: ;.
> +PASS (function (){ 'use strict'; delete someDeclaredGlobal;}) threw exception SyntaxError: Unexpected token: ;.
> +PASS (function(){(function (){ 'use strict'; delete someDeclaredGlobal;})}) threw exception SyntaxError: Unexpected token: ;.

These errors aren't very good -- we're saying that the semicolon is unexpected but it's actually strict mode disallowing deletion of an unqualified property

> LayoutTests/fast/js/basic-strict-mode-expected.txt:114
> +PASS 'use strict'; ++eval threw exception SyntaxError: Unexpected EOF (in strict mode).
> +PASS (function(){'use strict'; ++eval}) threw exception SyntaxError: Unexpected token: }.
> +PASS 'use strict'; eval++ threw exception SyntaxError: Unexpected token: ++.
> +PASS (function(){'use strict'; eval++}) threw exception SyntaxError: Unexpected token: ++.
> +PASS 'use strict'; --eval threw exception SyntaxError: Unexpected EOF (in strict mode).
> +PASS (function(){'use strict'; --eval}) threw exception SyntaxError: Unexpected token: }.
> +PASS 'use strict'; eval-- threw exception SyntaxError: Unexpected token: --.

These errors need to be improved:
"Can't assign to eval in strict mode"
and
"Can't assign to arguments in strict mode"

[Juan C. Montemayor]

Created attachment 97477 [details]
diff of the fast/js/ tests

[Juan C. Montemayor]

Created attachment 97479 [details]
fast/js tests diff

[Juan C. Montemayor]

Created attachment 97487 [details]
fast/js tests diff

[Juan C. Montemayor]

Created attachment 97503 [details]
fast/js tests diff

[Juan C. Montemayor]

Created attachment 97513 [details]
fast/js tests diff

[Juan C. Montemayor]

Created attachment 97519 [details]
fast/js tests diff

[Juan C. Montemayor]

Created attachment 97522 [details]
proposed patch

[Oliver Hunt]

Comment on attachment 97522 [details]
proposed patch

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

Almost there, just a few style issues remain (i'm only complaining about the other whitespace issues because the style errors will require you to update your patch anyway).  The only real problem is the additional branches added for eval/arguments in parseUnaryExpression (see my comments below)

Make sure that all the tests that don't fail before this patch still pass after it (i'm surprised that there aren't any non-js/sputnik tests that ended up testing parse error messages).

If you use "webkit-patch post" the style checker will run locally and catch any other commit-queue breaking style issues.

> Source/JavaScriptCore/parser/JSParser.cpp:85
> +    JSParser(Lexer*, JSGlobalData*, FunctionParameters*, bool isStrictContext, bool isFunction,  const SourceCode*);

Yay style error! 1 space not two (the commit queue barfs on all style errors)

> Source/JavaScriptCore/parser/JSParser.cpp:349
> +    ALWAYS_INLINE void updateErrorMessageSpecialCase(JSTokenType expectedToken) 
> +    {

This function should be doing early returns rather than assign->break->return

> Source/JavaScriptCore/parser/JSParser.cpp:425
> +        m_errorMessage = UString(msg);

This UString conversion should happen automatically

> Source/JavaScriptCore/parser/JSParser.cpp:512
> -
> +    

Adding whitespace

> Source/JavaScriptCore/parser/JSParser.cpp:933
> +    

Added whitespace (alas xcode will fight to insert this whitespace :-/ )

> Source/JavaScriptCore/parser/JSParser.cpp:1503
> +        failIfFalseIfStrictWithMessage(m_statementDepth == 1, "Function declarations cannot be in a nested block in strict mode");

You should ask geoff if he can come up with a better worded sentence than this -- it's somewhat clunky as is.

> Source/JavaScriptCore/parser/JSParser.cpp:1586
> +        failIfTrueWithMessage(*name == m_globalData->propertyNames->underscoreProto, "Can't name a function __proto__");

Should use Cannot or Can't consistently -- i'd lean towards Cannot

> Source/JavaScriptCore/parser/JSParser.cpp:2386
> -    bool isEvalOrArguments = false;
> +    bool isEval = false;
> +    bool isArguments = false;
>      if (strictMode() && !m_syntaxAlreadyValidated) {
>          if (context.isResolve(expr)) {
> -            isEvalOrArguments = *m_lastIdentifier == m_globalData->propertyNames->eval || *m_lastIdentifier == m_globalData->propertyNames->arguments;
> +            isEval = *m_lastIdentifier == m_globalData->propertyNames->eval;
> +            isArguments = *m_lastIdentifier == m_globalData->propertyNames->arguments;
>          }
>      }
> -    failIfTrueIfStrict(isEvalOrArguments && modifiesExpr);
> +    failIfTrueIfStrictWithMessage(isEval && modifiesExpr, "'eval' cannot be modified in strict mode");
> +    failIfTrueIfStrictWithMessage(isArguments && modifiesExpr, "'arguments' cannot be modified in strict mode");

These changes are unnecessary as you can pull the term 'eval' or 'arguments' out of m_lastIdentifier (we try not to add extra branches to valid code paths of the parser and lexer)

> Source/JavaScriptCore/parser/JSParser.cpp:2394
> +        failIfTrueIfStrictWithMessage(isEval, "'eval' cannot be modified in strict mode");
> +        failIfTrueIfStrictWithMessage(isArguments, "'arguments' cannot be modified in strict mode");

as above

> Source/JavaScriptCore/parser/JSParser.cpp:2404
> +        failIfTrueIfStrictWithMessage(isEval, "'eval' cannot be modified in strict mode");
> +        failIfTrueIfStrictWithMessage(isArguments, "'arguments' cannot be modified in strict mode");

And again

> Source/JavaScriptCore/parser/JSParser.h:37
> +class SourceCode;   

Added whitespace

> Source/JavaScriptCore/parser/JSParser.h:141
> +        

Moar whitespace

[Juan C. Montemayor]

Created attachment 97535 [details]
hopefully final patch

fixed the whitespace issues and the style errors.

fixed the branching problems

[Oliver Hunt]

Comment on attachment 97535 [details]
hopefully final patch

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

revert the project file change, did you verify all the tests pass now?  With that done i believe everything is finished

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2668
>  			};
>  			buildConfigurationList = 149C277108902AFE008A9EFC /* Build configuration list for PBXProject "JavaScriptCore" */;
>  			compatibilityVersion = "Xcode 3.1";
> -			developmentRegion = English;
>  			hasScannedForEncodings = 1;
>  			knownRegions = (
>  				English,

Revert your changes to the project file

> Source/JavaScriptCore/parser/JSParser.cpp:387
> +            errorMessage = "Unexpected EOF"; 
> +            m_errorMessage = errorMessage.impl();
> +            return;
> +        case RETURN:
> +            errorMessage = "Return statements are only valid inside functions";
> +            m_errorMessage = errorMessage.impl();

No reason to do these in two steps

[Juan C. Montemayor]

Created attachment 97606 [details]
definitely final patch

[Oliver Hunt]

Comment on attachment 97606 [details]
definitely final patch

r- as it doesn't look like you've updated all the test results :-( -- a quick scan implies that fast/dom fast/encoding fast/events fast/parser fast/regex html5lib, and the http tests all contain parse errors and i'd be surprised if we're doing anything to actual clobber what the reported error actually is.

[Juan C. Montemayor]

(In reply to comment #19)
> (From update of attachment 97606 [details])
> r- as it doesn't look like you've updated all the test results :-( 

oh right.. I was just thinking about fast/js and sputnik. I'll reset every test.

> -- a quick scan implies that fast/dom fast/encoding fast/events fast/parser fast/regex html5lib, and the http tests all contain parse errors and i'd be surprised if we're doing anything to actual clobber what the reported error actually is.

[Juan C. Montemayor]

Created attachment 97681 [details]
updated patch

added the new outputs for test cases. Some test case outputs were unchanged (like media-src-allowed) since that test fails regardless of the changes made by this patch.

[Juan C. Montemayor]

Created attachment 97682 [details]
updated patch

fixed a rouge whitespace...

[Darin Adler]

(In reply to comment #21)
> Some test case outputs were unchanged (like media-src-allowed) since that test fails regardless of the changes made by this patch.

Since these are regression tests, even a failure is checked against the expected output. So if the output changes, we do need to land new expectations. Unless the test is skipped because of an entry in the Skipped file.

So I am concerned by what you say above about not updating some test expected results files.

For the specific case of media-src-allowed, the expected results don’t seem to include any exceptions, so what you are reporting where that test fails may be a phenomenon limited to your computer.

[Darin Adler]

Comment on attachment 97682 [details]
updated patch

The EWS is unable to process your patch because something is wrong with the JSParser parts of the patch:

patching file Source/JavaScriptCore/parser/JSParser.cpp
patch: **** malformed patch at line 48: @@ -123,11 +135,16 @@ private:

patching file Source/JavaScriptCore/parser/JSParser.h
patch: **** malformed patch at line 29:  

I’m not sure exactly why that’s failing, but until you deal with that we don’t have any early-warning results for your patch.

[Oliver Hunt]

(In reply to comment #23)
> (In reply to comment #21)
> > Some test case outputs were unchanged (like media-src-allowed) since that test fails regardless of the changes made by this patch.
> 
> Since these are regression tests, even a failure is checked against the expected output. So if the output changes, we do need to land new expectations. Unless the test is skipped because of an entry in the Skipped file.
> 
> So I am concerned by what you say above about not updating some test expected results files.
> 
> For the specific case of media-src-allowed, the expected results don’t seem to include any exceptions, so what you are reporting where that test fails may be a phenomenon limited to your computer.

This is about tests that fail on his machine locally without his patch -- not about tests where the output has changed

[Oliver Hunt]

Comment on attachment 97682 [details]
updated patch

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

Noooooooooooooo -- r- due to those tests that have bogus changes (listed below)

> LayoutTests/fast/js/cyclic-proto-expected.txt:1
> +posix_spawn fatal error: 9!

This looks bogus

> LayoutTests/http/tests/media/pdf-served-as-pdf-expected.txt:-5
> +posix_spawn fatal error: 9!
> +FAIL: Timed out waiting for notifyDone to be called
>  EXPECTED (video.error == 'null') OK
>  EVENT(loadstart)
> -EVENT(error)
> -failed trying to load PDF file served as PDF OK
> -END OF TEST

bogus change

> LayoutTests/http/tests/media/reload-after-dialog-expected.txt:3
> +posix_spawn fatal error: 9!
> +FAIL: Timed out waiting for notifyDone to be called

Bogus change

[Juan C. Montemayor]

Created attachment 97684 [details]
updated patch

thought I reverted them all ;)

[Juan C. Montemayor]

Created attachment 97685 [details]
patch

sorry for the spam -- keep finding whitespace... I'm pretty sure it's good now.

[Oliver Hunt]

Comment on attachment 97685 [details]
patch

r=me!!!!!

[WebKit Review Bot]

Comment on attachment 97685 [details]
patch

Rejecting attachment 97685 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
le LayoutTests/sputnik/Conformance/13_Function_Definition/S13_A7_T3-expected.txt
patching file LayoutTests/sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/15.1.2/15.1.2.1_eval/S15.1.2.1_A2_T2-expected.txt
patching file LayoutTests/sputnik/Conformance/15_Native_Objects/15.3_Function/15.3.4/15.3.4.2_Function.prototype.toString/S15.3.4.2_A1_T1-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 2

Full output: http://queues.webkit.org/results/8914343

[Oliver Hunt]

(In reply to comment #30)
> (From update of attachment 97685 [details])
> Rejecting attachment 97685 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2
> 
> Last 500 characters of output:
> le LayoutTests/sputnik/Conformance/13_Function_Definition/S13_A7_T3-expected.txt
> patching file LayoutTests/sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/15.1.2/15.1.2.1_eval/S15.1.2.1_A2_T2-expected.txt
> patching file LayoutTests/sputnik/Conformance/15_Native_Objects/15.3_Function/15.3.4/15.3.4.2_Function.prototype.toString/S15.3.4.2_A1_T1-expected.txt
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 2
> 
> Full output: http://queues.webkit.org/results/8914343

I'll have to go over this with you tomorrow as for whatever reason we're getting a bogus patch.  Just as an exercise you could try creating a patch and seeing if that will produce a patch that works (eg. put it up in the bug with r?cq?)

[Oliver Hunt]

CCing eric as webkit-patch should not be producing invalid patches.

[Eric Seidel (no email)]

Invalid patches, oh noes!  dbates is our expert on svn-create-patch/svn-apply, etc.

[Daniel Bates]

(In reply to comment #33)
> Invalid patches, oh noes!  dbates is our expert on svn-create-patch/svn-apply, etc.

This patch (attachment 97685 [details]) is messed up. In particular, some of the chunk range lines are wrong. See bug #62965 for more details.

[Juan C. Montemayor]

Created attachment 97819 [details]
Patch

[Oliver Hunt]

(In reply to comment #35)
> Created an attachment (id=97819) [details]
> Patch

Yay looks like that patch is applying properly.

[Oliver Hunt]

Comment on attachment 97819 [details]
Patch

Clearing flags on attachment: 97819

Committed r89257: <http://trac.webkit.org/changeset/89257>

[Oliver Hunt]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It broke two javascriptcore tests on all bots:

SL : http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/30548/steps/jscore-test

GTK: http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/14871/steps/jscore-test

Qt: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/34367/steps/jscore-test

:((

Could you fix it, or should we roll it out?

[Csaba Osztrogonác]

I really don't understand why didn't you guys execute javascript-core-tests and run-webkit-tests and watch the bots after landing. As far as I know, committing regressions is unacceptable, but let me know if this policy is changed.

[Csaba Osztrogonác]

I won't rollout this regression, but if you would like to do it,
just set cq+ here: https://bugs.webkit.org/show_bug.cgi?id=63052

[Oliver Hunt]

(In reply to comment #40)
> I really don't understand why didn't you guys execute javascript-core-tests and run-webkit-tests and watch the bots after landing. As far as I know, committing regressions is unacceptable, but let me know if this policy is changed.

We do run tests, I just checked and this seems to be a release build specific crash, even the most rudimentary testing would have shown you this.  Of course i recognise that if you're not working on the actual engine you don't encounter issues like this normally.

[Oliver Hunt]

No need to reopen a bug unless the fix is rolled out

[Eric Seidel (no email)]

Seeing this posix_spawn fatal error: 9! message on the NRWT bot too:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(NRWT)/r90024%20(214)/fast/js/cyclic-prototypes-pretty-diff.html

I wonder what is causing this to bleed into tests.

[Gavin Barraclough]

*** Bug 25630 has been marked as a duplicate of this bug. ***

[Gavin Barraclough]

*** Bug 42181 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

<rdar://problem/5907896>