RESOLVED FIXED 190501
[JSC] Align stringification algorithm of the Function constructor with the spec
https://bugs.webkit.org/show_bug.cgi?id=190501
Summary [JSC] Align stringification algorithm of the Function constructor with the spec
Yusuke Suzuki
Reported 2018-10-11 17:15:59 PDT
[JSC] Change Function constructor's source to match to the spec and test262
Attachments
Patch (9.57 KB, patch)
2018-10-11 17:20 PDT, Yusuke Suzuki
no flags
Patch (10.37 KB, patch)
2018-10-11 17:36 PDT, Yusuke Suzuki
mark.lam: review+
ews-watchlist: commit-queue-
Archive of layout-test-results from ews100 for mac-sierra (2.46 MB, application/zip)
2018-10-11 18:27 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.58 MB, application/zip)
2018-10-11 20:02 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews114 for mac-sierra (3.23 MB, application/zip)
2018-10-11 20:27 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.38 MB, application/zip)
2018-10-11 22:42 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews202 for win-future (12.75 MB, application/zip)
2018-10-11 23:59 PDT, EWS Watchlist
no flags
Yusuke Suzuki
Comment 1 2018-10-11 17:20:29 PDT
Mark Lam
Comment 2 2018-10-11 17:28:46 PDT
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
Comment 3 2018-10-11 17:36:04 PDT
EWS Watchlist
Comment 4 2018-10-11 18:27:57 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 5 2018-10-11 18:27:58 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 6 2018-10-11 20:02:19 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 7 2018-10-11 20:02:20 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 8 2018-10-11 20:27:53 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 9 2018-10-11 20:27:55 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 10 2018-10-11 22:42:27 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 11 2018-10-11 22:42:28 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 12 2018-10-11 23:59:48 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 13 2018-10-11 23:59:59 PDT Comment hidden (obsolete)
Mark Lam
Comment 14 2018-10-12 09:08:58 PDT
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 15 2018-10-12 11:45:51 PDT
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
Comment 16 2023-09-27 13:18:09 PDT
Alexey Shvayka
Comment 17 2023-09-27 13:19:03 PDT
*** Bug 247437 has been marked as a duplicate of this bug. ***
Alexey Shvayka
Comment 18 2023-09-28 09:28:03 PDT
Alexey Shvayka
Comment 19 2023-09-28 09:29:06 PDT
*** Bug 251304 has been marked as a duplicate of this bug. ***
EWS
Comment 20 2023-09-29 00:40:40 PDT
Committed 268633@main (ff01344c4f58): <https://commits.webkit.org/268633@main> Reviewed commits have been landed. Closing PR #18362 and removing active labels.
Note You need to log in before you can comment on or make changes to this bug.