Bug 201897 – Syntax checker should report duplicate __proto__ properties

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

<rdar://problem/53201788>

[Tadeu Zagallo]

Created attachment 379022 [details]
Patch

[Mark Lam]

Comment on attachment 379022 [details]
Patch

For a change like this, can you run test262 to make sure there are no regressions?  I wonder what was the rationale for having a different parseStrictObjectLiteral() in the first place.

[Mark Lam]

Comment on attachment 379022 [details]
Patch

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

r=me if test262 passes.

> Source/JavaScriptCore/ChangeLog:14
> +        When syntax checking in sloppy mode, we call parseObjectLiteral, which does not fill the
> +        property names and therefore fails to detect the duplicate __proto__ properties. We replace
> +        parseObjectLiteral with the strict version since:
> +        - the only difference between parseObjectLiteral and parseStrictObjectLiteral seems to be
> +          whether we store the properties' names
> +        - parseObjectLiteral has to fallback to parseStrictObjectLiteral in the presence of getters/setters.

Ok, I think I understand the difference now.  parseObjectLiteral() passes TreeBuilder::DontBuildStrings to the Lexer, but parseStrictObjectLiteral() does not.  This appears to be an optimization that went wrong, because the parser relies on getting the token strings in order to do the redefined __proto__ check.  But since it told the lexer not to build this token (which it wrongly presumed is unnecessary work for the SyntaxChecker), the test fails.  It is safe to always use parseStrictObjectLiteral() instead because there is actually no semantic difference in terms of "strict" vs "sloppy": parseStrictObjectLiteral() is also misnamed.  Can you please put these details in the ChangeLog?

You should still run test262 to make sure there are no hidden surprises here.

[Tadeu Zagallo]

Thanks for the review. I checked test262 and JetStream2 and it seems all good. I'll update the ChangeLog before landing it.

[Tadeu Zagallo]

Created attachment 379148 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 379148 [details]
Patch for landing

Clearing flags on attachment: 379148

Committed r250098: <https://trac.webkit.org/changeset/250098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.