Bug 198630

Summary: Three checks are missing in Proxy internal methods
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Minor CC: commit-queue, darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://github.com/tc39/ecma262/pull/666
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Revert EXCEPTION_ASSERT change and add isExtensible test.
none
Patch
none
Archive of layout-test-results from ews211 for win-future
none
Patch none

Comment 1 Alexey Shvayka 2019-06-06 16:57:16 PDT
Created attachment 371538 [details]
Patch
Comment 2 Saam Barati 2019-06-06 17:07:15 PDT
Comment on attachment 371538 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        These checks are needed to maintain the invariants of the essential internal methods.

can you also list what exactly they are here?

> Source/JavaScriptCore/runtime/ProxyObject.cpp:666
> +        bool targetIsExtensible = target->isExtensible(exec);

can you add a test here where this does effects to ensure it's being tested? Also, is the ordering of these effects correct? Is this when this effect is supposed to happen in the spec?
Comment 3 EWS Watchlist 2019-06-06 18:37:55 PDT
Comment on attachment 371538 [details]
Patch

Attachment 371538 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12400876

New failing tests:
http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html
Comment 4 EWS Watchlist 2019-06-06 18:37:57 PDT
Created attachment 371546 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5
Comment 5 Alexey Shvayka 2019-06-06 18:50:59 PDT
Created attachment 371547 [details]
Patch
Comment 6 Alexey Shvayka 2019-06-06 19:04:42 PDT
(In reply to Saam Barati from comment #2)
> Comment on attachment 371538 [details]
> Patch
>
> can you add a test here where this does effects to ensure it's being tested?
> Also, is the ordering of these effects correct? Is this when this effect is
> supposed to happen in the spec?

I've updated the change log and replaced RETURN_IF_EXCEPTION with EXCEPTION_ASSERT (did I get it right?). All the checks (newly added and existent) are in the right place, according to spec.
Comment 7 Saam Barati 2019-06-06 19:54:37 PDT
Comment on attachment 371547 [details]
Patch

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

> Source/JavaScriptCore/runtime/ProxyObject.cpp:666
> +        bool targetIsExtensible = target->isExtensible(exec);

Since this is observable, I was asking for a test where you assert that this happens and with the proper ordering. (e.g, you can observe this when "target" is another proxy)
Comment 8 Alexey Shvayka 2019-06-13 22:32:23 PDT
Created attachment 372107 [details]
Revert EXCEPTION_ASSERT change and add isExtensible test.
Comment 9 EWS Watchlist 2019-06-14 00:28:29 PDT
Comment on attachment 372107 [details]
Revert EXCEPTION_ASSERT change and add isExtensible test.

Attachment 372107 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12471585

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline
apiTests
Comment 10 Saam Barati 2019-06-14 11:03:15 PDT
Comment on attachment 372107 [details]
Revert EXCEPTION_ASSERT change and add isExtensible test.

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

> JSTests/stress/proxy-delete.js:98
> +            throw new Error("should not be called if [[GetOwnProperty]] returns undefined");

can we also add a test where we ensure it gets called in the right order with respect to other effects?
Comment 11 Alexey Shvayka 2019-06-14 17:30:35 PDT
Created attachment 372155 [details]
Patch

Add another isExtensible order test.
Comment 12 EWS Watchlist 2019-06-14 21:19:17 PDT
Comment on attachment 372155 [details]
Patch

Attachment 372155 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12479417

New failing tests:
storage/indexeddb/modern/get-keyrange.html
storage/indexeddb/index-cursor.html
Comment 13 EWS Watchlist 2019-06-14 21:19:20 PDT
Created attachment 372174 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 14 Alexey Shvayka 2019-06-18 16:17:15 PDT
Comment on attachment 372155 [details]
Patch

Failed tests seem irrelevant.
Comment 15 Alexey Shvayka 2019-07-24 15:25:15 PDT
Created attachment 374819 [details]
Patch

Add info about this feature shipping in other engines.
Comment 16 WebKit Commit Bot 2019-07-24 19:09:48 PDT
Comment on attachment 374819 [details]
Patch

Clearing flags on attachment: 374819

Committed r247811: <https://trac.webkit.org/changeset/247811>
Comment 17 WebKit Commit Bot 2019-07-24 19:09:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2019-07-24 19:10:24 PDT
<rdar://problem/53524130>