1054835 - JSMSG_PAREN_BEFORE_FORMAL parse error in Method Definitions in strict mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2124.0 Safari/537.36

Steps to reproduce:

js> a={a(){}}
({a:function a(){}})
js> a={a(){"use strict"}}
typein:205:5 SyntaxError: missing ( before formal parameters:
typein:205:5 a={a(){"use strict"}}
typein:205:5 .....^

Comment 1

[ziyunfei]

Attachment: bug1054835.patch

This probably isn't the right way to fix this problem.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8484815 [details] [diff] [review]
bug1054835.patch

Review of attachment 8484815 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review, ziyunfei.

You're right, this isn't the best fix. :)

The problem is this code at Parser::functionDef():

  2057	        tokenStream.seek(start);
  2058	        if (funName && tokenStream.getToken() == TOK_ERROR)
  2059	            return null();

What this means is:  "After seeking, if funName is not null, call getToken() to skip the next token, and return null if getToken() fails."

This is clearly silly; we shouldn't have to skip a token. We should arrange for 'start' to contain the exact position we want to seek to. The main reason we haven't already done this is premature optimization: we're trying to use a single TokenStream::Position to deal with both arrow functions (in assignExpr()) and strict mode (in functionDef()). We use rewinding for both; it saves some stack space to pass a reference to the same Position object from assignExpr() to functionDef().

We shouldn't really be using rewinding at all. While we're doing it, I think we should tune the code for clarity and correctness. I'm going to take this and mop up some of the mess here.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: JSMSG_PAREN_BEFORE_FORMAL parse error in Method Definitions in strict mode. Tests by ziyunfei, who discovered the bug

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8493382 [details] [diff] [review]
JSMSG_PAREN_BEFORE_FORMAL parse error in Method Definitions in strict mode. Tests by ziyunfei, who discovered the bug

Review of attachment 8493382 [details] [diff] [review]:
-----------------------------------------------------------------

Seems right, but I didn't think on it super-super-deeply.  Relying on fuzzers to come up with any garbage seems the most effective use of time for utmost assurance here.

Comment 5

[Jason Orendorff [:jorendorff]]

Oops, I forgot to delete the code that was actually causing the problem:

>diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp
>--- a/js/src/frontend/Parser.cpp
>+++ b/js/src/frontend/Parser.cpp
>@@ -2058,8 +2058,6 @@ Parser<ParseHandler>::functionDef(Handle
>         directives = newDirectives;
> 
>         tokenStream.seek(start);
>-        if (funName && tokenStream.getToken() == TOK_ERROR)
>-            return null();
> 
>         // functionArgsAndBody may have already set pn->pn_body before failing.
>         handler.setFunctionBody(pn, null());

Comment 6

[Jason Orendorff [:jorendorff]]

Looks better now: https://tbpl.mozilla.org/?tree=Try&rev=7a7d1ec8b5f6

Comment 7

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/962f8d56981f

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/962f8d56981f