755821 - Function() should use the parser's argument parsing code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[:Benjamin Peterson]

Function() currently uses the tokenizer to parse function parameters. This means there are two implementations of functionArgument parsing, and parameters defined with Function don't support argument destructuring. Parser should expose its argument parsing interface and Function should use it.

Comment 1

[Jim Blandy :jimb]

JSFunction::toString could be simplified if the Function constructor prepended the argument list to the function's source code. Comments in the patch for bug 761723 seem to suggest that should be done as part of this work.

Comment 2

[Nicholas Nethercote [inactive]]

Has this been fixed elsewhere?

Comment 3

[:Benjamin Peterson]

No. Function() still tokenizes the arguments itself.

Comment 4

[a.d.bergi]

Not only destructuring doesn't work, parameter initializers ("default values") cause a SyntaxError as well (reported at http://stackoverflow.com/q/31429405/1048572).

Comment 6

[Tom S [:evilpie]]

Attachment: very raw WIP

I am actually surprised I got this working at all, but it seems to work. I used Chrome's way of inserting /**/ after the parameter list to ensure that we really parse the parameters as parameters.

Comment 7

[Tom S [:evilpie]]

Attachment: rebased WIP

I have no idea what I am doing here, so this might be better suited for somebody with some parser experience. The attached WIP makes this work `new Function("...a", "return a;")`.

Comment 8

[Tom S [:evilpie]]

For some reason this doesn't throw: `new Function("}}}}")`

Comment 9

[Tom S [:evilpie]]

(In reply to Tom Schuster [:evilpie] from comment #8)
> For some reason this doesn't throw: `new Function("}}}}")`
I fixed this.

The only remaining test failures with the shell are related to the changed toString() representation. (Includes that /**/ sentinel)

Comment 10

[Tom S [:evilpie]]

Attachment: WIP v2 new new Function parsing

So some things to note:
- It seems like it's easier to have the parser start parsing after an imaginary `function anonymous(`, that seems rather weird.
- I adopted Chrome's idea of adding "\n/**/" after the parameter list. I think this is supposed to prevent people from doing funny stuff with the parameter list. As in adding code that parses differently together with ") {" after the parameter list. This is however not totally foolproof, new Function("/*", "return 1"); should probably not parse, but passes in Chrome and my implementation.
- Line numbers are now +1.

Comment 11

[Jason Orendorff [:jorendorff]]

How about this, as an alternative to adding "\n/**/":

-   When building the string, remember the offset of the end of the argument list.

-   Pass it through to Parser<>::functionArgsAndBodyGeneric (optional argument)

-   After functionArguments() succeeds, if an offset was passed, and the current pos() doesn't match it,
    throw a SyntaxError.

Things to test:

- can't comment out the implicit ") {" using either kind of comment
- can't use a backtick to make the implicit ") {" part of a template string
- can't fool the parser by including "){" in an argument,
  making the implicit ") {" part of the body of the function
- can't fool the parser by passing an argument like "= function (",
  making the implicit ") {" part of a default argument expression

Comment 12

[Tom S [:evilpie]]

Attachment: WIP using offsets

Great idea Jason! Now the only difference in parsing seems to be that the debugger now observers a different source including the parameter list with onNewScript. Not sure what to do there.

Comment 13

[Jason Orendorff [:jorendorff]]

How about putting "function anonymous(" in the string, for the sole purpose of including them in the Source?

We can just manually step the tokenStream over those tokens before beginning to parse.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8738151 [details] [diff] [review]
WIP using offsets

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

The general approach seems fine to me. I will look more carefully with r?

As for the Debugger reporting a different source, do you mean different than what it is now, or different from some other method on the same function like toString()? If it's the former, I'm inclined to agree with jorendorff's suggestion of prepending "function anonymous(" for the sake of the source.

Comment 15

[Tom S [:evilpie]]

Following Jason's advice of adding "function anonymous(" I have to fix various things that expect the old format especially Function#toString.

Comment 16

[Jason Orendorff [:jorendorff]]

Function#toString is what we were fixing. I hoped you'd be able to ditch the code to compute funCon in FunctionToString(). This would be good, because that code is gibberish. :-) Then you should be able to ditch all that buildBody stuff.

I recommended it because the code *should* get a lot simpler! If that's not the case, ignore me!

Comment 17

[Tom S [:evilpie]]

I don't understand how I can add some bit of information to JSScript, JSFunction or something else so that we don't prefix the toString result with function + name. I don't fully grok the interaction with lazy scripts, cloning and such. If somebody just adds that flag for me :)

Comment 18

[Jason Orendorff [:jorendorff]]

If you upload your latest patch and ni?me, I'll give it a shot, though I'm certain to feel stupid once I see what the problem is...

Comment 19

[Tom S [:evilpie]]

Attachment: WIP v2 Sketch a fix for toString

Here you go. Thinking about this some more made me realize that putting this bit on JSScript is probably not good enough, because asm.js also implements their own toString. I think asm.js doesn't even use JSScripts so we need to store the wasFunctionConstructor bit somewhere else.

Comment 20

[Jason Orendorff [:jorendorff]]

Status: What I want is for FunctionToString to do almost nothing in most circumstances. It should just have to take a slice out of the source. That's all.

It's not hard... but it's a bit of a pain to implement because the frontend code around function parsing is so complicated (lazy parsing, strict mode, asm.js, and lots of other little things happen at function boundaries).

So I've been spending spare hours on this and it's getting close. If I can't get it done Monday or Tuesday, I'll just post whatever I've got and give up on it.

Comment 21

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #20)
> It's not hard... but it's a bit of a pain to implement because the frontend
> code around function parsing is so complicated (lazy parsing, strict mode,
> asm.js, and lots of other little things happen at function boundaries).

*And* there are lots of different entry points into function parsing: function statements, function expressions, arrow functions, legacy generator-expressions, the new parseStandaloneFunction.

To be clear, all I'm trying to achieve is correct source code coordinates for functions. A third complication is that these are stored in multiple places and even (in FunctionBox) in two different forms, senselessly.

Comment 22

[Jason Orendorff [:jorendorff]]

This is still like my 8th priority but I've been making steady enough progress on this I decided to hold on to it for another week. If it stalls I'll give up, I promise :)

Comment 23

[Jason Orendorff [:jorendorff]]

Attachment: , part 2 - Change toString/toSource to use exact source code only. (WIP. Fails several tests.)

MozReview-Commit-ID: AEp3PvZHODq

Comment 24

[Jason Orendorff [:jorendorff]]

Attachment: , part 2 - Change toString/toSource to use exact source code only. Passes tests

MozReview-Commit-ID: AEp3PvZHODq

Comment 25

[Jason Orendorff [:jorendorff]]

Attachment: , part 2 - Change toString/toSource to use exact source code only. Passes tests

MozReview-Commit-ID: AEp3PvZHODq

Comment 26

[Jason Orendorff [:jorendorff]]

All right, this at least passes the JS tests and jit-tests, and it's -160 lines of code!

Pretty sure there's still a bug or two, *and*:

*   The new argStart stuff is not great. It would be better for the lazy-parsing code in
    CompileLazyFunction to just be able to cope with the few syntax variations that can occur
    at the beginning of a lazy function.

*   After this change there are some offset fields in asmjs code that are no longer used;
    they should be dropped entirely.

Comment 27

[Jason Orendorff [:jorendorff]]

In case it wasn't clear I am dropping this with an audible thud --- good luck.

Comment 28

[Jason Orendorff [:jorendorff]]

Computed names make it a bit difficult to get rid of argStart/argBegin. :-|

Comment 29

[Tom S [:evilpie]]

Attachment: WIP v3 Use normal parsing code fore new Function

This still has at least one crash with basic/destructuring-default.js. Something about a HashTable from newVarScopeData().

Oh and we are back to the prefixing with "function[*] anonymous" problem: Not prefixing gives the right column numbers when reporting SyntaxErrors, but bad results for script-sources in the debugger.

Anyway, I have no real idea if what I am doing in Parser.cpp is correct, I just kind of copied parts from standaloneFunctionBody and standaloneLazyFunction. It does seem a lot of stuff got simpler since the rewrite \o/

Comment 30

[Tom S [:evilpie]]

Hey shu, do you mind running jit-test/tests/basic/destructuring-default.js with the previous patch applied and see if you can spot some obvious bug? Most of the Parser::standaloneFunction code I just copy-pasted from other Praser code :/

Comment 31

[Shu-yu Guo [:shu]]

Attachment: Clean up: make ParseContext::VarScope less of a footgun.

Comment 32

[Shu-yu Guo [:shu]]

Comment on attachment 8786442 [details] [diff] [review]
WIP v3 Use normal parsing code fore new Function

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

I posted a patch that makes ParseContext::VarScope less of a footgun and actually null out pc->varScope_ on destruction.

::: js/src/frontend/Parser.cpp
@@ +2151,5 @@
> +        return null();
> +
> +    fn->pn_pos.end = pos().end;
> +
> +    if (!finishFunction())

The bug is that finishFunction() expects there to be a valid varScope_ on ParseContext. For parsing functions, we make an additional VarScope if there are parameter expressions. Otherwise we re-use the function scope. In the case of destructuring with parameter exprs, we stack-allocated a new VarScope inside functionFormalParametersAndBody above. So functionFormalParametersAndBody already calls its own finishFunction(). The reason the old standaloneFunction called finishFunction separately is because it did *not* call functionFormalParametersAndBody since it parsed its own parameters.

Comment 33

[Tom S [:evilpie]]

I am still not finished rebasing, but I do have to say that part 2 is really a lot of changes across the whole codebase.

Comment 34

[Tom S [:evilpie]]

Attachment: Part 2 - (Incorrect) Rebase

I am pretty sure I switched argsStart, begin and end somewhere, but I have to find it. I rebased this by hand :/. The Function constructor works though :). Running jit-test we run it a lot of "this token was previously looked up with a different modifier, potentially making tokenization non-deterministic" asserts.

Comment 35

[Tom S [:evilpie]]

Attachment: Rebased again even more failures ...

It seems like for lazy function that are reparsed into real functions somehow don't get the right begin/end, but still parse? Maybe just something with source data.

Test case:
function g(x) {
    if (x) {
        function f() {}
        return f;
    }
}
print(g(1) + "");

There are some other random failures as well.

JIT test failures:

arrow-functions/syntax-errors.js
asm.js/bug1268955-usestrict-semantics.js
asm.js/bug1268955-usestrict-semantics.js
asm.js/bug941877.js
asm.js/testSource.js
asm.js/testSource.js
asm.js/testSource.js
basic/bug1141154.js
basic/destructuring-default.js
basic/function-tosource-lambda.js
basic/iterable-error-messages.js
basic/testBug961969.js
basic/withSourceHook.js
debug/Debugger-debuggees-28.js
debug/Frame-eval-21.js
debug/Frame-implementation-01.js
debug/Frame-onStep-resumption-05.js
debug/Memory-takeCensus-08.js
debug/Object-parameterNames.js
debug/RematerializedFrame-retval.js
debug/Script-gc-02.js
debug/Script-gc-03.js
debug/Script-sourceStart-02.js
debug/bug1282741.js
debug/resumption-08.js
for-of/decompiler.js
ion/bug1233343.js
parser/modifier-arrow-rest.js
parser/syntax-error-illegal-character.js
parser/modifier-regexp-vs-div.js
parser/modifier-yield-without-operand-1.js
parser/modifier-yield-without-operand-2.js
wasm/import-export.js
wasm/import-export.js
xdr/asm.js
xdr/lazy.js

Comment 36

[Tom S [:evilpie]]

Attachment: WIP v4 Use normal parsing code fore new Function

Okay success, we are pretty close. jstests passed and jit-tests has the same error 5 times plus the one token error described earlier. I must not be setting the correct end position somewhere. Jason can you take a look? I probably just forgot to adapt one line of your patch somewhere again. I also added a test.
Shu should probably review this patch, because Jason basically wrote 90% of it. (I am going to adjust authorship accordingly) 

I changed the jsapi.cpp CompileFunction to set the name after parsing, because XUL seems to pass function names that aren't valid identifiers like "root-element_XBL_Constructor".

There is another assert during browser startup (plus one about before <= end agin):
0x00007fffea0a6a49 in AssertScopeMatchesEnvironment (scope=0x7fffc1c4a040, originalEnv=0x7fffbcd94520) at /home/tom/projects/inbound/js/src/vm/Stack.cpp:145
145	                MOZ_ASSERT(env->as<LexicalEnvironmentObject>().isGlobal());


Failures:
arrow-functions/syntax-errors.js
basic/destructuring-default.js
basic/testBug961969.js
debug/Memory-takeCensus-08.js
wasm/import-export.js
--wasm-always-baseline wasm/import-export.js

Comment 37

[Tom S [:evilpie]]

Attachment: WIP v5 Use normal parsing code fore new Function

Fixed the scope issue and disabled the assertion in newExprStatement and we get  a working browser.

Comment 38

[Shu-yu Guo [:shu]]

Comment on attachment 8801529 [details] [diff] [review]
WIP v5 Use normal parsing code fore new Function

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

I'm reading through this still.

Could you explain why argsStart needs to be separated from bufStart? I couldn't quite figure from the code how they're different.

Comment 39

[Tom S [:evilpie]]

Sorry for giving up :(

Comment 40

[Tooru Fujisawa [:arai]]

iiuc, [bufStart:bufEnd) contains whole function source, that is the result of fun.toString().
and [argsStart:bufEnd) contains the part that needs to be parsed when compiling lazy function.

storing [bufStart:argsStart) range helps generating better .toString() result.
before the patch, we generate that part depending on flags, but it means some details are not reflected to .toString() result,
such as, comment, computed property, spaces.

Comment 41

[Tooru Fujisawa [:arai]]

(In reply to Tom Schuster [:evilpie] from comment #10)
> - Line numbers are now +1.

If we follow this proposal and use the generated code for compiling, column number of 1st line is also changed.
  https://tc39.github.io/Function-prototype-toString-revision/

before patch:
  Function("a()")() => error at Function:1:1

with WIP patch above:
  Function("a()")() => error at Function:2:1

with the proposal's syntax applied:
  Function("a()")() => error at Function:2:4

this breaks line/column numbering of resource://gre/modules/workers/require.js
with the patch, line number doesn't match between error message and require'd file...

Comment 42

[Tooru Fujisawa [:arai]]

so, the question is, how many cases depend on the current line/column numbering of Function ctor.

Comment 43

[Tooru Fujisawa [:arai]]

FWIW, chrome returns 2:1 for Function("a()")()

Comment 44

[Tooru Fujisawa [:arai]]

Also, from the change to JS::CompileFunction, event handlers are also affected by line number +1,

Comment 45

[Tooru Fujisawa [:arai]]

taking over.

I'm about to fix only Function constructor's parameter handling here, leaving toString,
since it's touched by proposal, and currently it doesn't match to any implementation.
we should wait for a while before touching the code.
  https://tc39.github.io/Function-prototype-toString-revision/
  https://github.com/tc39/Function-prototype-toString-revision/issues/20

also, leaving that part to other bug will make the patch simpler :)

about line numbering I'll use line number 0 as beginning of the function, so that function body starts from line 1.

Comment 46

[Tooru Fujisawa [:arai]]

Attachment: Parse arguments of Function constructor properly.

So, here's simplified version of the previous patch, that only touches Function constructor's parameter handling and source code storage.

Changes from previous patch:

  * Changed the following properties back to original meaning
      * ParseNode.pn_pos.begin for PNK_FUNCTION
      * FunctionBox.bufStart
      * AsmJSMetadata.srcStart
      * JSScript.sourceStart_
      * Lazyscript.begin_

    so that this patch doesn't affect existing code that touches them

  * Reverted all parts that touches Function#toString for function that is not
    Function constructor

  * Changed |BytecodeCompiler::compileFunctionBody| to receive
    the function source that starts with parameter, not "function".
    This is for reusing existing toString.
    and also removed |argsStart| since it's always the beginning

  * Stored |parameterListEnd| to |ScriptSource|
    Added |ScriptSource::functionBodyString| too get function body
    in debugger

  * Changed some places that calls |JS::CompileFunction| to use line = 0
    instead of line = 1 in |CompileOptions|, to make the function body starts
    from line 1

  * Renamed |lambdaParen| parameter of js::FunctionToString to |noAddParenToLambda|,
    just because |lambdaParen| sounds wrongs for purpose of the parameter


Also, summary of other changes from previous patch:

  * Generate source in FunctionConstructor and JS::CompileFunction,
    from passed parameters and body

  * Check |parameterListEnd| position after parsing parameter, and throw if
    it doesn't match, to check strange syntax in Function ctor parameters

  * Changed function names not to include "Body"
     * BytecodeCompiler::compileFunctionBody => BytecodeCompiler::compileStandaloneFunction
      * frontend::CompileFunctionBody => frontend::CompileStandaloneFunction
      * frontend::CompileStarGeneratorBody => frontend::CompileStandaloneGenerator
      * frontend::CompileAsyncFunctionBody => frontend::CompileStandaloneAsyncFunction
      * Parser::standaloneFunctionBody => Parser::standaloneFunction

  * Removed the following properties/methods, since no more used:
      * BytecodeCompiler::setSourceArgumentsNotIncluded
      * BytecodeCompiler.sourceArgumentsNotIncluded
      * ScriptSource::argumentsNotIncluded
      * ScriptSource.argumentsNotIncluded_

  * Updated tests to follow new behavior

Comment 47

[Tooru Fujisawa [:arai]]

remaining concern is that, since I used |line = 0| for the Function's first line, sometimes users and tools may see line 0 in error and debugger.
it already happens in devtools/client/inspector/markup/test/browser_markup_events3.js

but that is less impact than using |line = 1|, that causes all line numbering different, especially in |require()| and event handler.

Comment 48

[Jason Orendorff [:jorendorff]]

Thanks for taking. I had been working through bugs apparently introduced by rebasing, but wasn't really getting anywhere.

If V8 uses |line = 1|, I'm for that (a separate patch is fine, just in case).

Comment 49

[Tooru Fujisawa [:arai]]

to be clear, |require()| uses Function ctor, and event handler uses JS::CompileFunction.
so, the situation is a bit different.

we could apply some trick to JS::CompileFunction, to make body starts with line 1 for event handlers,
or perhaps we could provide another API for event handlers and something that expects function body starts from line 1.


but |require()| will be directly affected by |line = 1| change,

https://dxr.mozilla.org/mozilla-central/rev/28e2a6dde76ab6ad4464a3662df1bd57af04398a/toolkit/components/workerloader/require.js#137-140
>        let code = new Function("exports", "require", "module",
>          source + "\n//# sourceURL=" + uri + "\n"
>        );
>        code(exports, require, module);

if we add the API above, we might be able to use it there, but I'm not sure if it worth extra complicity around APIs.

Comment 50

[Shu-yu Guo [:shu]]

Comment on attachment 8811051 [details] [diff] [review]
Parse arguments of Function constructor properly.

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

Not yet finished, posting what I have as I'll finish the rest on a different computer.

::: dom/events/EventListenerManager.cpp
@@ +1070,5 @@
>    if (NS_WARN_IF(!GetOrCreateDOMReflector(cx, aElement, &v))) {
>      return NS_ERROR_FAILURE;
>    }
>    JS::CompileOptions options(cx);
> +  // Use line 0 to make the function body starts from line 1.

Oof this worries me. I wonder how we can tell the extent (if any) of this breaking the web.

::: js/src/frontend/BytecodeCompiler.cpp
@@ -676,5 @@
>      return bce.emitFunctionScript(pn->pn_body);
>  }
>  
> -// Compile a JS function body, which might appear as the value of an event
> -// handler attribute in an HTML <INPUT> tag, or in a Function() constructor.

How come this comment was removed? Is it no longer true? I'd like the comment to remain.

::: js/src/frontend/Parser.cpp
@@ -2319,5 @@
> -    for (uint32_t i = 0; i < formals.length(); i++) {
> -        if (!notePositionalFormalParameter(fn, formals[i], false, &duplicatedParam))
> -            return null();
> -    }
> -    funbox->hasDuplicateParameters = duplicatedParam;

\o/

@@ +3354,5 @@
>  bool
>  Parser<ParseHandler>::functionFormalParametersAndBody(InHandling inHandling,
>                                                        YieldHandling yieldHandling,
> +                                                      Node pn, FunctionSyntaxKind kind,
> +                                                      uint32_t parameterListEnd)

Could you use a Maybe<uint32_t> for parameterListEnd instead of 0 as a sentinel value to make the intention clear?

@@ +3387,5 @@
>          }
>      }
>  
> +    // When parsing something for new Function() we have to make sure to
> +    // only treat a certain part of the source as a paremeter list.

Typo: parameter

@@ +3448,5 @@
>          if (!matched) {
>              error(JSMSG_CURLY_AFTER_BODY);
>              return false;
>          }
> +        funbox->bufEnd = pos().end;

Is it the case that the last token is always '}', and so pos().end == pos().begin + 1?

::: js/src/jsapi.cpp
@@ +4205,5 @@
>  /*
>   * enclosingScope is a static enclosing scope, if any (e.g. a WithScope).  If
>   * the enclosing scope is the global scope, this must be null.
>   *
>   * enclosingDynamicScope is a dynamic scope to use, if it's not the global.

While you're here, could you change "static enclosing scope" to "scope" and "dynamic scope" to "environment"? Thanks!

@@ +4251,5 @@
> +static MOZ_MUST_USE bool
> +BuildFunctionString(unsigned nargs, const char* const* argnames,
> +                    const SourceBufferHolder& srcBuf, StringBuffer* out,
> +                    uint32_t* parameterListEnd)
> +{

MOZ_ASSERT(parameterListEnd) and MOZ_ASSERT(out) please.

Comment 51

[Shu-yu Guo [:shu]]

Comment on attachment 8811051 [details] [diff] [review]
Parse arguments of Function constructor properly.

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

I'm still kind of worried about the line number changes for event handlers. Who knows what people depend on...

I don't think we have a better way to find out breakage than to just try it, however.

::: js/src/jit-test/tests/debug/Script-gc-02.js
@@ +9,5 @@
>  
>  gc();
>  
>  for (var i = 0; i < arr.length; i++)
> +    assertEq(arr[i].lineCount, 3);

As an aside, it's kind of weird we have such a test.

::: js/src/jsfun.cpp
@@ +1693,2 @@
>  
> +    if (!sb.append(") {\n"))

See note above for putting these constants into static consts.

@@ +1717,5 @@
>       * anonymous function in the top-level scope that its constructor inhabits.
>       * Thus 'var x = 42; f = new Function("return x"); print(f())' prints 42,
>       * and so would a call to f from another top-level's script or function.
>       */
>      RootedAtom anonymousAtom(cx, cx->names().anonymous);

Please double check that a |new Function|'d function cannot recursively call itself using "anonymous". I'm pretty sure this is already the case.

@@ -1892,5 @@
> -        }
> -    }
> -
> -    if (hasRest)
> -        fun->setHasRest();

\o/

::: js/src/jsscript.cpp
@@ +1681,5 @@
> +    MOZ_ASSERT(isFunctionBody());
> +
> +    // +4 for ") {\n"
> +    size_t start = parameterListEnd_ + 4;
> +    // -2 for "\n}"

Please put these string constants ") {\n" and "\n}" into static consts somewhere and derive the lengths from those instead of hardcoding lengths and re-typing the constants.

Comment 52

[Tooru Fujisawa [:arai]]

Thank you for reviewing.

(In reply to Shu-yu Guo [:shu] from comment #50)
> @@ +3448,5 @@
> >          if (!matched) {
> >              error(JSMSG_CURLY_AFTER_BODY);
> >              return false;
> >          }
> > +        funbox->bufEnd = pos().end;
> 
> Is it the case that the last token is always '}', and so pos().end ==
> pos().begin + 1?

Yes


(In reply to Shu-yu Guo [:shu] from comment #51)
> @@ +1717,5 @@
> >       * anonymous function in the top-level scope that its constructor inhabits.
> >       * Thus 'var x = 42; f = new Function("return x"); print(f())' prints 42,
> >       * and so would a call to f from another top-level's script or function.
> >       */
> >      RootedAtom anonymousAtom(cx, cx->names().anonymous);
> 
> Please double check that a |new Function|'d function cannot recursively call
> itself using "anonymous". I'm pretty sure this is already the case.

This patch doesn't touch that part.
I'll try fixing it in bug 636635.

Comment 53

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc201de2d391256b2c978120158fe2bfb6d8b41
Bug 755821 - Parse arguments of Function constructor properly. r=shu

Comment 54

[Tooru Fujisawa [:arai]]

The remaining parts about toString should be fixed in bug 1317400.

Comment 55

[Tooru Fujisawa [:arai]]

I forgot to support single line comment in parameter, that's because we currently don't put newline after parameter list.
I'll fix it here.
maybe, by limiting the buffer length while parsing parameter.

Comment 56

[Tooru Fujisawa [:arai]]

discussed with shu on IRC, and the single line comment should be fixed at the same time as toString (bug 1317400)

Comment 57

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0dc201de2d39