Bug 210862 - Remove revoked Proxy checks from ProxyCreate
Summary: Remove revoked Proxy checks from ProxyCreate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-22 09:08 PDT by Alexey Shvayka
Modified: 2020-04-23 22:44 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.83 KB, patch)
2020-04-22 09:26 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2020-04-22 10:36 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (13.37 KB, patch)
2020-04-23 21:55 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 Alexey Shvayka 2020-04-22 09:08:04 PDT
Remove revoked Proxy checks from ProxyCreate
Comment 1 Alexey Shvayka 2020-04-22 09:26:35 PDT
Created attachment 397205 [details]
Patch
Comment 2 Alexey Shvayka 2020-04-22 10:36:46 PDT
Created attachment 397222 [details]
Patch

Adjust tests.
Comment 3 Ross Kirsling 2020-04-22 12:50:10 PDT
Comment on attachment 397222 [details]
Patch

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

> JSTests/ChakraCore.yaml:1849
> -  cmd: runChakra :skip, "NoException", "arraywithproxy.baseline", []
> +  cmd: runChakra :skipDueToOutdatedOrBadTest, "NoException", "", []

Whoooa, I didn't know this setting existed. Pretty sure I could have used it in the past! :D

> Source/JavaScriptCore/ChangeLog:11
> +        Also cleans up ProxyObject creation by using isFunction() instead of
> +        isCallable(), which are identical.

Hmm, I wonder if this should be cleaned up for redundancy (in another patch of course)...

It's odd that isCallable still checks getCallData for JSFunctionType, when it doesn't look like this could ever be CallType::None.
Comment 4 Alexey Shvayka 2020-04-23 11:29:18 PDT
Comment on attachment 397222 [details]
Patch

(In reply to Ross Kirsling from comment #3)

Thank you for review!

> Whoooa, I didn't know this setting existed. Pretty sure I could have used it
> in the past! :D

I've discovered it by accident: it was used for /test/es6/proxyenumbug.js, which is totally outdated. JSC made some significant progress on spec compliance since ChakraCore tests were last updated, and I believe tests are up to date with ECMA-262 and don't have print('key' + Symbol()) anymore. It would be great to re-sync them.

> It's odd that isCallable still checks getCallData for JSFunctionType, when
> it doesn't look like this could ever be CallType::None.

This would be a great improvement! With this patch, isCallable() has 1-2 call sites as isFunction() seems to be preferred. Also there are a few usages of getCallData() which ignore the CallData.

Do you mind if I land this after test262 sync?
Comment 5 Alexey Shvayka 2020-04-23 21:55:45 PDT
Created attachment 397425 [details]
Patch

Set reviewer and rebase patch.
Comment 6 EWS 2020-04-23 22:43:37 PDT
Committed r260621: <https://trac.webkit.org/changeset/260621>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397425 [details].
Comment 7 Radar WebKit Bug Importer 2020-04-23 22:44:23 PDT
<rdar://problem/62282842>