...
Created attachment 371809 [details] Patch
Comment on attachment 371809 [details] Patch Attachment 371809 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12440321 New failing tests: stress/regexp-match-proxy.js.ftl-eager-no-cjit-b3o1 stress/regexp-replace-proxy.js.ftl-no-cjit-validate-sampling-profiler stress/regexp-match-proxy.js.ftl-no-cjit-b3o0 stress/regexp-replace-proxy.js.no-ftl stress/regexp-replace-proxy.js.ftl-eager-no-cjit stress/regexp-replace-proxy.js.bytecode-cache stress/regexp-match-proxy.js.dfg-eager stress/regexp-match-proxy.js.dfg-eager-no-cjit-validate stress/regexp-match-proxy.js.ftl-no-cjit-no-inline-validate stress/regexp-replace-proxy.js.no-cjit-collect-continuously stress/regexp-replace-proxy.js.ftl-no-cjit-small-pool stress/regexp-replace-proxy.js.default stress/regexp-replace-proxy.js.no-llint stress/regexp-match-proxy.js.dfg-maximal-flush-validate-no-cjit stress/regexp-replace-proxy.js.dfg-eager stress/regexp-match-proxy.js.mini-mode stress/regexp-match-proxy.js.bytecode-cache stress/regexp-match-proxy.js.no-cjit-validate-phases stress/regexp-match-proxy.js.no-cjit-collect-continuously stress/regexp-match-proxy.js.ftl-no-cjit-no-put-stack-validate stress/regexp-replace-proxy.js.ftl-eager stress/regexp-match-proxy.js.no-ftl stress/regexp-replace-proxy.js.ftl-eager-no-cjit-b3o1 stress/regexp-replace-proxy.js.dfg-eager-no-cjit-validate stress/regexp-replace-proxy.js.ftl-no-cjit-no-inline-validate stress/regexp-match-proxy.js.ftl-no-cjit-validate-sampling-profiler stress/regexp-replace-proxy.js.no-cjit-validate-phases stress/regexp-replace-proxy.js.dfg-maximal-flush-validate-no-cjit stress/regexp-match-proxy.js.ftl-eager-no-cjit stress/regexp-match-proxy.js.no-llint stress/regexp-match-proxy.js.default stress/regexp-replace-proxy.js.ftl-no-cjit-b3o0 stress/regexp-match-proxy.js.ftl-eager stress/regexp-replace-proxy.js.mini-mode stress/regexp-match-proxy.js.ftl-no-cjit-small-pool stress/regexp-replace-proxy.js.ftl-no-cjit-no-put-stack-validate apiTests
Comment on attachment 371809 [details] Patch Attachment 371809 [details] did not pass jsc-armv7-ews (jsc-only): Output: https://webkit-queues.webkit.org/results/12440898 New failing tests: stress/regexp-replace-proxy.js.no-cjit-collect-continuously stress/regexp-match-proxy.js.no-cjit-collect-continuously stress/regexp-replace-proxy.js.dfg-eager-no-cjit-validate stress/regexp-replace-proxy.js.default stress/regexp-match-proxy.js.no-cjit-validate-phases stress/regexp-replace-proxy.js.no-cjit-validate-phases stress/regexp-match-proxy.js.no-llint stress/regexp-match-proxy.js.dfg-maximal-flush-validate-no-cjit stress/regexp-match-proxy.js.default stress/regexp-replace-proxy.js.dfg-eager stress/regexp-replace-proxy.js.mini-mode stress/regexp-match-proxy.js.mini-mode stress/regexp-match-proxy.js.dfg-eager stress/regexp-replace-proxy.js.dfg-maximal-flush-validate-no-cjit stress/regexp-match-proxy.js.dfg-eager-no-cjit-validate stress/regexp-replace-proxy.js.no-llint apiTests
Created attachment 371843 [details] Patch
Comment on attachment 371843 [details] Patch Attachment 371843 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12444183 New failing tests: http/tests/security/contentSecurityPolicy/navigate-self-to-data-url.html
Created attachment 371847 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 371843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371843&action=review Nice catch. Can you add a test for this? > Source/JavaScriptCore/runtime/ProxyObject.cpp:483 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); Use `false` instead of `encodedJSValue()`. > Source/JavaScriptCore/runtime/ProxyObject.cpp:485 > + throwVMTypeError(exec, scope, makeString("Proxy object's 'set' trap returned falsy value for property '", String(propertyName.uid()), "'")); When throwing an error, we should also `return` here. if (!success && slot.isStrictMode()) { throwVMTypeError(exec, scope, makeString("Proxy object's 'set' trap returned falsy value for property '", String(propertyName.uid()), "'")); return false; } > Source/JavaScriptCore/runtime/ProxyObject.cpp:486 > + RELEASE_AND_RETURN(scope, success); Just returning success is OK because `success` part won't throw an error. So, `return success`.
Created attachment 371864 [details] Patch
Thanks for thorough review, Yusuke. It is a bit unusual to handle JS abrupt completions while remembering to early return in native code manually and not accidentally override JS exception. I will be adding tests for this case to Test262: there are quite a few operators that do [[Set]] and I would need test generation tooling to cover them all (w/o too much repetition).
(In reply to Alexey Shvayka from comment #9) > Thanks for thorough review, Yusuke. > It is a bit unusual to handle JS abrupt completions while remembering to > early return in native code manually and not accidentally override JS > exception. > I will be adding tests for this case to Test262: there are quite a few > operators that do [[Set]] and I would need test generation tooling to cover > them all (w/o too much repetition). You can test it by returning `false` from Proxy's [[Set]] operation and checking the error is thrown. Actually we modified regexp-match-proxy.js etc., because this patch changes the behavior of that tests. So you can craft the test by using the similar way done in regexp-match-proxy.js. While we have test262 for spec compatibility checking, we also would like to have a test in JSC's JSTests/ to (1) avoid accidentally causing the regression in very implementation detailed way, and to (2) ensure the current JSC's assuming behavior even after the spec is changed :)
Created attachment 371877 [details] Patch
(In reply to Yusuke Suzuki from comment #10) > While we have test262 for spec compatibility checking, we also would like to > have a test in JSC's JSTests/ to (1) avoid accidentally causing the > regression in very implementation detailed way, and to (2) ensure the > current JSC's assuming behavior even after the spec is changed :) That makes perfect sense, thank you for detailed response. Also, stress tests (in hot loop) are out of Test262's scope.
Comment on attachment 371877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371877&action=review Looks nice > JSTests/stress/proxy-set.js:99 > + proxy.x = 40; Can you add a test for indexed access case? `proxy[42] = 40`. I think it will not throw an error right now. > Source/JavaScriptCore/runtime/ProxyObject.cpp:487 > + if (!success && slot.isStrictMode()) { > + throwVMTypeError(exec, scope, makeString("Proxy object's 'set' trap returned falsy value for property '", String(propertyName.uid()), "'")); > + return false; > + } Now, I think that we should move this check inside performPut. performPut is used by putByIndex case too. Currently, we are missing putByIndex case because we have a check here. Inside performPut, we have `trapResultAsBool` condition. We should throw an error if it is done in the strict mode (passing shouldThrow parameter).
Created attachment 371906 [details] Patch
Comment on attachment 371906 [details] Patch r=me
Comment on attachment 371906 [details] Patch Clearing flags on attachment: 371906 Committed r246346: <https://trac.webkit.org/changeset/246346>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51652957>