code: const { async, } = { }; actual result syntax error expected result: pass "," is considered as a part of method definition, and just throws syntax error. this breaks existing code, per https://discourse.mozilla-community.org/t/anyone-else-has-ff-52-issues/14253 [Tracking Requested - why for this release]: breaks existing valid JS code

Parser::propertyName treats "async" as non-keyword if it won't become an async function syntax and become a valid property syntax. we should exclude TOK_COMMA there too.

Comment on attachment 8845553 [details] [diff] [review] Handle shorthand property and destructuring with async keyword properly. Review of attachment 8845553 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +9304,5 @@ > TokenKind tt; > if (!tokenStream.getToken(&tt)) > return null(); > + if (tt != TOK_LP && tt != TOK_COLON && tt != TOK_RC && tt != TOK_ASSIGN && > + tt != TOK_COMMA) I don't understand why this check is in the negative. That is, looking at the |switch (ltok)| below, we can handle the case of the next token being TOK_NUMBER, TOK_LB, TOK_STRING, or TokenKindIsPossibleIdentifierName(ltok). ISTM we should check those directly.

Comment on attachment 8845553 [details] [diff] [review] Handle shorthand property and destructuring with async keyword properly. Review of attachment 8845553 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +9310,5 @@ > isAsync = true; > ltok = tt; > } else { > tokenStream.ungetToken(); > } To fix bug 1345968 at the same time for property names, we can do something like: if (tt == TOK_NUMBER || tt == TOK_LB || tt == TOK_STRING || TokenKindIsPossibleIdentifierName(tt)) { isAsync = true; tokenStream.consumeKnownToken(tt); ltok = tt; }

Thanks :) fixed

Comment on attachment 8845722 [details] [diff] [review] Handle shorthand property and destructuring with async keyword properly. Review of attachment 8845722 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/60728dc4728b6ec0eea3be80ed285f53b67fab61 Bug 1345960 - Handle shorthand property and destructuring with async keyword properly. r=shu

almost same patch is applicable for mozilla-aurora Approval Request Comment > [Feature/Bug causing the regression]: Bug 1185106 > [User impact if declined]: Breaks existing valid JS code (throws syntax error for unrelated syntax to bug 1185106) > [Is this code covered by automated tests?]: Yes > [Has the fix been verified in Nightly?]: Not yet > [Needs manual test from QE? If yes, steps to reproduce]: No > [List of other uplifts needed for the feature/fix]: No > [Is the change risky?]: Less risky > [Why is the change risky/not risky?]: Allows a syntax that was rejected wrongly, and it was working with the expected path before the patch. > [String changes made/needed]: None

There's some difference in beta, because of the change in bug 1336783 (Removes KeywordIsName and adds TokenKindIsPossibleIdentifierName). can you review that part?

Tracking for all branches based on the comment that this breaks existing valid JS code.

Comment on attachment 8845745 [details] [diff] [review] (mozilla-beta) Handle shorthand property and destructuring with async keyword properly. beta needs different patch. I'll post a patch for release/esr52 that is also a bit different Approval Request Comment > [Feature/Bug causing the regression]: Bug 1185106 > [User impact if declined]: Breaks existing valid JS code (throws syntax error for unrelated syntax to bug 1185106) > [Is this code covered by automated tests?]: Yes > [Has the fix been verified in Nightly?]: Yes > [Needs manual test from QE? If yes, steps to reproduce]: No > [List of other uplifts needed for the feature/fix]: None > [Is the change risky?]: Less risky > [Why is the change risky/not risky?]: Allows a syntax that was rejected wrongly, and it was working with the expected path before the patch. > [String changes made/needed]: None

for release Approval Request Comment > [Feature/Bug causing the regression]: Bug 1185106 > [User impact if declined]: Breaks existing valid JS code (throws syntax error for unrelated syntax to bug 1185106) > [Is this code covered by automated tests?]: Yes > [Has the fix been verified in Nightly?]: Yes > [Needs manual test from QE? If yes, steps to reproduce]: No > [List of other uplifts needed for the feature/fix]: None > [Is the change risky?]: Less risky > [Why is the change risky/not risky?]: Allows a syntax that was rejected wrongly, and it was working with the expected path before the patch. > [String changes made/needed]: None ==== for esr52 [Approval Request Comment] > If this is not a sec:{high,crit} bug, please state case for ESR consideration: not security, but breaks the web and extensions. > User impact if declined: Breaks existing valid JS code. > Fix Landed on Version: 55 > Risk to taking this patch (and alternatives if risky): Less risky, this allows a syntax that was rejected wrongly, and it was working with the expected path before the patch. > String or UUID changes made by this patch: None

Comment on attachment 8845733 [details] [diff] [review] (mozilla-aurora) Handle shorthand property and destructuring with async keyword properly. r=shu Fix a JS issue. Aurora54+.

Comment on attachment 8845745 [details] [diff] [review] (mozilla-beta) Handle shorthand property and destructuring with async keyword properly. webcompat fix for beta53

Comment on attachment 8846231 [details] [diff] [review] (mozilla-release/mozilla-esr52) Handle shorthand property and destructuring with async keyword properly. r=shu webcompat fix for release and esr52

https://hg.mozilla.org/releases/mozilla-esr52/rev/3833e945e3cd (FIREFOX_ESR_52_0_X_RELBRANCH - 52.0.2)