WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(10.37 KB, patch)
2018-10-11 17:36 PDT
,
Yusuke Suzuki
mark.lam
: review+
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
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
Details
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
Details
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2018-10-11 17:20:29 PDT
Created
attachment 352109
[details]
Patch
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
Created
attachment 352112
[details]
Patch
EWS Watchlist
Comment 4
2018-10-11 18:27:57 PDT
Comment hidden (obsolete)
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
Comment 5
2018-10-11 18:27:58 PDT
Comment hidden (obsolete)
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 6
2018-10-11 20:02:19 PDT
Comment hidden (obsolete)
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
Comment 7
2018-10-11 20:02:20 PDT
Comment hidden (obsolete)
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 8
2018-10-11 20:27:53 PDT
Comment hidden (obsolete)
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
Comment 9
2018-10-11 20:27:55 PDT
Comment hidden (obsolete)
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 10
2018-10-11 22:42:27 PDT
Comment hidden (obsolete)
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
Comment 11
2018-10-11 22:42:28 PDT
Comment hidden (obsolete)
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 12
2018-10-11 23:59:48 PDT
Comment hidden (obsolete)
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
Comment 13
2018-10-11 23:59:59 PDT
Comment hidden (obsolete)
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
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
<
rdar://102065151
>
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
Pull request:
https://github.com/WebKit/WebKit/pull/18362
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.
Top of Page
Format For Printing
XML
Clone This Bug