Bug 177398 - JSC should throw if proxy set returns falsish in strict mode context
Summary: JSC should throw if proxy set returns falsish in strict mode context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-22 16:19 PDT by Filip Pizlo
Modified: 2019-06-11 20:19 PDT (History)
14 users (show)

See Also:


Attachments
Patch (1.92 KB, patch)
2019-06-10 19:35 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (4.74 KB, patch)
2019-06-11 07:27 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.08 MB, application/zip)
2019-06-11 08:35 PDT, Build Bot
no flags Details
Patch (4.73 KB, patch)
2019-06-11 12:37 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (5.70 KB, patch)
2019-06-11 14:09 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (8.54 KB, patch)
2019-06-11 18:41 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-09-22 16:19:15 PDT
...
Comment 1 Alexey Shvayka 2019-06-10 19:35:15 PDT
Created attachment 371809 [details]
Patch
Comment 2 Build Bot 2019-06-10 22:04:52 PDT
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 3 jsc-armv7 EWS 2019-06-11 00:14:51 PDT
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
Comment 4 Alexey Shvayka 2019-06-11 07:27:12 PDT
Created attachment 371843 [details]
Patch
Comment 5 Build Bot 2019-06-11 08:35:41 PDT
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
Comment 6 Build Bot 2019-06-11 08:35:43 PDT
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 7 Yusuke Suzuki 2019-06-11 12:18:31 PDT
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`.
Comment 8 Alexey Shvayka 2019-06-11 12:37:43 PDT
Created attachment 371864 [details]
Patch
Comment 9 Alexey Shvayka 2019-06-11 12:46:41 PDT
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).
Comment 10 Yusuke Suzuki 2019-06-11 13:00:51 PDT
(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 :)
Comment 11 Alexey Shvayka 2019-06-11 14:09:12 PDT
Created attachment 371877 [details]
Patch
Comment 12 Alexey Shvayka 2019-06-11 14:12:04 PDT
(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 13 Yusuke Suzuki 2019-06-11 17:37:00 PDT
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).
Comment 14 Alexey Shvayka 2019-06-11 18:41:53 PDT
Created attachment 371906 [details]
Patch
Comment 15 Yusuke Suzuki 2019-06-11 19:48:26 PDT
Comment on attachment 371906 [details]
Patch

r=me
Comment 16 WebKit Commit Bot 2019-06-11 20:18:58 PDT
Comment on attachment 371906 [details]
Patch

Clearing flags on attachment: 371906

Committed r246346: <https://trac.webkit.org/changeset/246346>
Comment 17 WebKit Commit Bot 2019-06-11 20:19:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2019-06-11 20:19:26 PDT
<rdar://problem/51652957>