Bug 190501 – [JSC] Align stringification algorithm of the Function constructor with the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

[JSC] Change Function constructor's source to match to the spec and test262

[Yusuke Suzuki]

Created attachment 352109 [details]
Patch

[Mark Lam]

Comment on attachment 352109 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] Change Function constructor's source to match to the spec and test262

Can you quote the relevant spec url in the ChangeLog comment below?

[Yusuke Suzuki]

Created attachment 352112 [details]
Patch

[EWS Watchlist]

Comment on attachment 352112 [details]
Patch

Attachment 352112 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9544406

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-4.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-2.html
js/dom/script-start-end-locations.html
http/tests/security/window-named-proto.html
http/tests/security/xssAuditor/regress-167121.html
js/dom/function-names.html
js/dom/parse-error-external-script-in-new-Function.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-1.html
inspector/debugger/search-scripts.html
fast/events/event-function-toString.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-3.html
inspector/console/messageAdded-from-named-evaluations.html

[EWS Watchlist]

Created attachment 352117 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352112 [details]
Patch

Attachment 352112 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9545474

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-4.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-2.html
js/dom/script-start-end-locations.html
http/tests/security/window-named-proto.html
http/tests/security/xssAuditor/regress-167121.html
js/dom/function-names.html
js/dom/parse-error-external-script-in-new-Function.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-1.html
inspector/debugger/search-scripts.html
fast/events/event-function-toString.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-3.html
inspector/console/messageAdded-from-named-evaluations.html

[EWS Watchlist]

Created attachment 352127 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352112 [details]
Patch

Attachment 352112 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9545404

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-4.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-2.html
js/dom/script-start-end-locations.html
http/tests/security/window-named-proto.html
http/tests/security/xssAuditor/regress-167121.html
js/dom/function-names.html
js/dom/parse-error-external-script-in-new-Function.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-1.html
fast/events/event-function-toString.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-3.html
inspector/console/messageAdded-from-named-evaluations.html

[EWS Watchlist]

Created attachment 352131 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352112 [details]
Patch

Attachment 352112 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9546666

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-4.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-2.html
js/dom/script-start-end-locations.html
http/tests/security/window-named-proto.html
http/tests/security/xssAuditor/regress-167121.html
js/dom/function-names.html
js/dom/parse-error-external-script-in-new-Function.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-1.html
fast/events/event-function-toString.html
imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-3.html

[EWS Watchlist]

Created attachment 352137 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 352112 [details]
Patch

Attachment 352112 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9547704

New failing tests:
fast/events/event-function-toString.html
http/tests/security/window-named-proto.html
js/dom/script-start-end-locations.html
js/dom/parse-error-external-script-in-new-Function.html
js/dom/function-names.html

[EWS Watchlist]

Created attachment 352144 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Mark Lam]

The mac-debug bot is reporting these failures:

Regressions: Unexpected text-only failures (11)
  fast/events/event-function-toString.html [ Failure ]
  http/tests/security/window-named-proto.html [ Failure ]
  http/tests/security/xssAuditor/regress-167121.html [ Failure ]
  imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-1.html [ Failure ]
  imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-2.html [ Failure ]
  imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-3.html [ Failure ]
  imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/window-onerror-with-cross-frame-event-listeners-4.html [ Failure ]
  inspector/console/messageAdded-from-named-evaluations.html [ Failure ]
  js/dom/function-names.html [ Failure ]
  js/dom/parse-error-external-script-in-new-Function.html [ Failure ]
  js/dom/script-start-end-locations.html [ Failure ]

I think some of them are legitimate given that you're changing the function toString() output in this patch.  Can you take a look?  I'll take a look at the patch now.

[Mark Lam]

Comment on attachment 352112 [details]
Patch

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

r=me if you fix the remaining EWS bot failures (I think at least some of them may be valid).

> Source/JavaScriptCore/ChangeLog:10
> +        We should have a line terminator after the function parameters to parse the parameters endings with `//`.
> +        We also remove a space after `,`. This is tested in test262 and our stress tests. We also note that this
> +        behavior is the same to V8.

This is not quite accurate (see my other comment below).  I suggest saying something like this instead:

"We should insert a line terminator after the last function parameter so that a parameter list like "a,b,c//" will not fail to parse.  The spec does not forbid the insertion of a line terminator before the close paren, and this is how Chrome and FF behaves.  The spec does forbid the insertion of line terminators after any other parameters except for the last because it explicit states that the parameters shall be separated only by a single ",".  Hence, we should also remove the space after the comma.  This is tested in test262 and our stress tests."

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:132
> +        // To meet this condition, what we can do is putting a line terminator after the parameter text P for single line comment (//) to work correctly.

/putting/put/.

> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:139
> +        builder.appendLiteral("\n) {\n");

There's something very hacky about this solution.  Here's why:

1. I don't see anything in the spec that says that a newline is expected just before the ')'.

2. While this matches Chrome and FireFox behavior, both Chrome and Firefox aren't handling this that much better.  Consider the following cases:

    Case 1:
    var f = new Function("a", "b", "c//", "return a + b + c"); // Parses successfully on Chrome and Firefox.

    f.toString(); // prints 
    // function anonymous(a,b,c//
    // ) {
    // return a + b + c
    // }"

    f(1, 2, 3); // Executes correctly on Chrome and Firefox, and returns 6.

    Case 2:
    var g = new Function("a", "b//", "c", "return a + b + c"); // Parses successfully on Chrome and Firefox.

    g.toString(); // prints 
    // function anonymous(a,b//,c
    // ) {
    // return a + b + c
    // }"

    g(1, 2, 3); // Chrome fails with: Uncaught ReferenceError: c is not defined at eval (eval at <anonymous> (newtab?ie=UTF-8:1), <anonymous>:3:16)
                     // Firefox fails with: ReferenceError: c is not defined

Why should the Function constructor be allowed to take a // comment in the last argument, but not any of the preceding ones?  It feels very inconsistent.  After reading the spec, I think I know why they do this:
    19.2.1.1.1 - 15.d.ii specifies that "nextArgString be ? ToString(nextArg)", and ToString here has specific semantics where the whole string of nextArg is returned (in the bad case, includes the // comment).
    19.2.1.1.1 - 15.d.iii specifies that"Set P to the string-concatenation of the previous value of P, "," (a comma), and nextArgString." which means we don't have the leeway to insert a newLine between arguments.

Hence, the only place where we can append a newline is after the last argument since that part is not specified.  Based on all these, I agree that this solution is correct, though ugly. 

I wish we can append that newLine only if needed e.g.

     x = new Function(); // Don't make this one look ugly: toString() renders "function anonymous()  { }"
     y = new Function("a//", ""); // toString() renders the ugly version with the appended newline, or one with comments removed: "function anonymous(a)  { }"

However, the downside of a pretty toString() render is that the web may already have made assumptions based on Chrome/FF behavior even though it's supposed to be implementation specific.  It's better for web compatibility to behave similarly.  Plus your implementation is simpler (less chance of bugs creeping in).

So, r=me.

[Alexey Shvayka]

<rdar://102065151>

[Alexey Shvayka]

*** Bug 247437 has been marked as a duplicate of this bug. ***

[Alexey Shvayka]

Pull request: https://github.com/WebKit/WebKit/pull/18362

[Alexey Shvayka]

*** Bug 251304 has been marked as a duplicate of this bug. ***

[EWS]

Committed 268633@main (ff01344c4f58): <https://commits.webkit.org/268633@main>

Reviewed commits have been landed. Closing PR #18362 and removing active labels.