Bug 63339 – JSON errors should be informative

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Oliver Hunt]

Currently the JSON parser just gives a generic "invalid json" style error for all errors, which isn't exactly helpful for debugging.

[Juan C. Montemayor]

Created attachment 101695 [details]
Work in progress

Some error messages don't make sense right now, but they will be improved on soon.

[Oliver Hunt]

Comment on attachment 101695 [details]
Work in progress

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

r-, due to some code style issues and error message clarity.   Would be good to also store the value of m_ptr for error feedback, then we can give line and column information.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:240
> +                m_lexErrorMessage = "JSON Lexer error: Single quotes are not allowed in JSON";

Call out the specific character as well just to make it clear

> Source/JavaScriptCore/runtime/LiteralParser.cpp:281
> +                m_lexErrorMessage = "JSON Lexer error: Unexpected end of string";

Unterminated string

> Source/JavaScriptCore/runtime/LiteralParser.cpp:325
> +                            m_lexErrorMessage = "JSON Lexer error: Escaped unicode is not a ASCII hexadecimal digit";

I would say "\\u..." is not a valid unicode escape

> Source/JavaScriptCore/runtime/LiteralParser.cpp:436
> +            m_lexErrorMessage = "JSON Lexer error: Exponent symbols should be followed by '+' or '-' and least one number";

+/- are optional

> Source/JavaScriptCore/runtime/LiteralParser.cpp:608
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse value expression";

At this point you either have a bogus token -- so the lexer should have an error message, or you should be able to say what token you got and why it is wrong.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:638
> +                m_parseErrorMessage = "JSON Parse error: Expected end of JSON but file continued";

Unexpected content at end of JSON literal.

> Source/JavaScriptCore/runtime/LiteralParser.h:97
> +            UString m_lexErrorMessage;

make it private

> Source/JavaScriptCore/runtime/LiteralParser.h:112
> +                : m_lexErrorMessage()

UString has a default constructor so this isn't necessary

[Oliver Hunt]

Comment on attachment 101695 [details]
Work in progress

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

> Source/JavaScriptCore/runtime/LiteralParser.cpp:541
> +                    m_parseErrorMessage = "JSON Parse error: Expected string in object expression";

Property name must be a string literal

> Source/JavaScriptCore/runtime/LiteralParser.cpp:626
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse name in expression";

name?  JSON statement must begin with a [, number, string, or (

That said no one will ever see this error message as you only hit this path when we're trying speculative parsing, not with true JSON.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:633
> +                if (m_lexer.currentToken().type != TokRParen) {
> +                    m_parseErrorMessage = "JSON Parse error: Expected right parenthesis after name in expression";

name again?

[Juan C. Montemayor]

(In reply to comment #3)
> (From update of attachment 101695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101695&action=review
> 
> > Source/JavaScriptCore/runtime/LiteralParser.cpp:541
> > +                    m_parseErrorMessage = "JSON Parse error: Expected string in object expression";
> 
> Property name must be a string literal
> 
> > Source/JavaScriptCore/runtime/LiteralParser.cpp:626
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse name in expression";
> 
> name?  JSON statement must begin with a [, number, string, or (
> 
> That said no one will ever see this error message as you only hit this path when we're trying speculative parsing, not with true JSON.
> 
> > Source/JavaScriptCore/runtime/LiteralParser.cpp:633
> > +                if (m_lexer.currentToken().type != TokRParen) {
> > +                    m_parseErrorMessage = "JSON Parse error: Expected right parenthesis after name in expression";
> 
> name again?

I'll fix these changes. I wrote down name since json.org said that "an object is an unordered set of name/value pairs" but I like property name much better!

[Juan C. Montemayor]

Created attachment 101791 [details]
Proposed patch

Updated code with Oliver's comments.

[Oliver Hunt]

Comment on attachment 101791 [details]
Proposed patch

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

> Source/JavaScriptCore/runtime/LiteralParser.cpp:339
> +                    m_lexErrorMessage = "JSON Lexer error: Unable to read string";

Invalid escape character %c

> Source/JavaScriptCore/runtime/LiteralParser.cpp:346
> +        m_lexErrorMessage = "JSON Lexer error: Unterminated string";

No lexer, only parse errors.

"JSON Parse error: "  is repeated everywhere, i think you should just drop that prefix, and add it at the end when you're making the error message.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:436
> +            m_lexErrorMessage = "JSON Lexer error: Exponent symbols should be followed by an optional '+' or '-' and then by at least one number";

Should just be JSON Parse error -- the lexer vs. parser difference isn't really relevant to develoeprs

> Source/JavaScriptCore/runtime/LiteralParser.cpp:634
> +                    case TokRBracket:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse right bracket";
> +                        return JSValue();
> +                    case TokRBrace:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse right brace";
> +                        return JSValue();
> +                    case TokIdentifier:
> +                        m_parseErrorMessage = String::format("JSON Parse error: Could not parse \"%s\"", UString(m_lexer.currentToken().stringToken).ascii().data()).impl();
> +                        return JSValue();
> +                    case TokColon:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse colon";
> +                        return JSValue();
> +                    case TokLParen:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse left parenthesis";
> +                        return JSValue();
> +                    case TokRParen:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse right parenthesis";
> +                        return JSValue();
> +                    case TokComma:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse comma";
> +                        return JSValue();
> +                    case TokDot:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse dot";
> +                        return JSValue();
> +                    case TokAssign:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse assignment";
> +                        return JSValue();
> +                    case TokSemi:
> +                        m_parseErrorMessage = "JSON Parse error: Could not parse semicolon";
> +                        return JSValue();

These error messages are icky, it should be something along the lines of "Unexpected token <blah>"

> Source/JavaScriptCore/runtime/LiteralParser.cpp:635
> +                    case TokEnd:

This should be unexpected EOF or some such.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:657
> +                        m_parseErrorMessage = "JSON statement must begin with a [, number, string, or (";

I'd rather go with unexpected token <blah>

> Source/JavaScriptCore/runtime/LiteralParser.cpp:664
> +                    m_parseErrorMessage = "JSON Parse error: Expected right parenthesis after property name in expression";

This error is bogus.  It's testing to ensure that you have balanced the parenthesis.  However you can't actually test for it as this path is only taken when preparsing content in the hope it's json.

[Juan C. Montemayor]

> > Source/JavaScriptCore/runtime/LiteralParser.cpp:634
> > +                    case TokRBracket:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse right bracket";
> > +                        return JSValue();
> > +                    case TokRBrace:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse right brace";
> > +                        return JSValue();
> > +                    case TokIdentifier:
> > +                        m_parseErrorMessage = String::format("JSON Parse error: Could not parse \"%s\"", UString(m_lexer.currentToken().stringToken).ascii().data()).impl();
> > +                        return JSValue();
> > +                    case TokColon:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse colon";
> > +                        return JSValue();
> > +                    case TokLParen:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse left parenthesis";
> > +                        return JSValue();
> > +                    case TokRParen:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse right parenthesis";
> > +                        return JSValue();
> > +                    case TokComma:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse comma";
> > +                        return JSValue();
> > +                    case TokDot:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse dot";
> > +                        return JSValue();
> > +                    case TokAssign:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse assignment";
> > +                        return JSValue();
> > +                    case TokSemi:
> > +                        m_parseErrorMessage = "JSON Parse error: Could not parse semicolon";
> > +                        return JSValue();
> 
> These error messages are icky, it should be something along the lines of "Unexpected token <blah>"
> 
> 
> > Source/JavaScriptCore/runtime/LiteralParser.cpp:657
> > +                        m_parseErrorMessage = "JSON statement must begin with a [, number, string, or (";
> 
> I'd rather go with unexpected token <blah>

the problem with doing this is that doing something like:

m_parseErrorMessage = String::format("Unexpected token \'%s\'", UString(m_lexer.currentToken().stringToken).ascii().data()).impl();

causes some regressiosn, since m_lexer.currentToken().stringToken may be null. Do you want me to write a special case for that? or write a const error message?

> 
> > Source/JavaScriptCore/runtime/LiteralParser.cpp:664
> > +                    m_parseErrorMessage = "JSON Parse error: Expected right parenthesis after property name in expression";
> 
> This error is bogus.  It's testing to ensure that you have balanced the parenthesis.  However you can't actually test for it as this path is only taken when preparsing content in the hope it's json.

So I should just leave this one out?

[Oliver Hunt]

> m_parseErrorMessage = String::format("Unexpected token \'%s\'", UString(m_lexer.currentToken().stringToken).ascii().data()).impl();
> 
> causes some regressiosn, since m_lexer.currentToken().stringToken may be null. Do you want me to write a special case for that? or write a const error message?

You'll want to special case it for each token type.

> 
> > 
> > > Source/JavaScriptCore/runtime/LiteralParser.cpp:664
> > > +                    m_parseErrorMessage = "JSON Parse error: Expected right parenthesis after property name in expression";
> > 
> > This error is bogus.  It's testing to ensure that you have balanced the parenthesis.  However you can't actually test for it as this path is only taken when preparsing content in the hope it's json.
> 
> So I should just leave this one out?

Yeah

[Juan C. Montemayor]

Created attachment 101797 [details]
proposed patch

patch, take two

[Oliver Hunt]

Comment on attachment 101797 [details]
proposed patch

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

Seems like one more round is necessary

> Source/JavaScriptCore/runtime/LiteralParser.cpp:499
> +                    m_parseErrorMessage = "Expected right bracket at end of array expression";

expected ]

> Source/JavaScriptCore/runtime/LiteralParser.cpp:519
> +                        m_parseErrorMessage = "Expected colon before value in object definition";

Expected ':' before value in object property definition

> Source/JavaScriptCore/runtime/LiteralParser.cpp:529
> +                    m_parseErrorMessage = "Expected right brace at the end of object definition";

'}'

> Source/JavaScriptCore/runtime/LiteralParser.cpp:548
> +                    m_parseErrorMessage = "Expected colon before value in object definition";

':'

> Source/JavaScriptCore/runtime/LiteralParser.cpp:564
> +                    m_parseErrorMessage = "Expected right brace at end of object expression";

'}'

> Source/JavaScriptCore/runtime/LiteralParser.cpp:668
> +                        m_parseErrorMessage = String::format("Could not parse \"%s\"", UString(m_lexer.currentToken().stringToken).ascii().data()).impl();

Unexpected identifier "%s"

[Juan C. Montemayor]

Created attachment 101805 [details]
proposed patch

patch, take three

[WebKit Review Bot]

Comment on attachment 101805 [details]
proposed patch

Attachment 101805 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9232276

New failing tests:
fast/canvas/webgl/framebuffer-test.html

[Juan C. Montemayor]

Oliver, do you know why the chrome-bot failed?

[Juan C. Montemayor]

Created attachment 101885 [details]
updated patch for cr-bot

hopefully the bot will like this one

[Juan C. Montemayor]

So, the cr-linux bot liked the last patch... but it failed 3 times before it passed. Is that significant?

[Adam Barth]

> So, the cr-linux bot liked the last patch... but it failed 3 times before it passed. Is that significant?

Nope.  That just means there were some existing failures unrelated to your patch.

[WebKit Review Bot]

Comment on attachment 101885 [details]
updated patch for cr-bot

Clearing flags on attachment: 101885

Committed r91686: <http://trac.webkit.org/changeset/91686>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

rolled out patch as it introduced 1.5% regression

[Juan C. Montemayor]

Created attachment 101968 [details]
patch (no speed regression this time)

This patch does not cause a speed regression in the sunspider tests. The cause for the regression in the previous patch was the line that created an error message for an unexpected identifier using String::format(). Removing the String::format from that line will still create an adequate error message (Unexpected identifier) and it will not cause a speed regression.

[WebKit Review Bot]

Comment on attachment 101968 [details]
patch (no speed regression this time)

Clearing flags on attachment: 101968

Committed r91765: <http://trac.webkit.org/changeset/91765>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.