Bug 198630 - Three checks are missing in Proxy internal methods
Summary: Three checks are missing in Proxy internal methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Nobody
URL: https://github.com/tc39/ecma262/pull/666
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-06 16:46 PDT by Alexey Shvayka
Modified: 2019-07-24 19:10 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.53 KB, patch)
2019-06-06 16:57 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.51 MB, application/zip)
2019-06-06 18:37 PDT, Build Bot
no flags Details
Patch (5.93 KB, patch)
2019-06-06 18:50 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Revert EXCEPTION_ASSERT change and add isExtensible test. (6.69 KB, patch)
2019-06-13 22:32 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2019-06-14 17:30 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.54 MB, application/zip)
2019-06-14 21:19 PDT, Build Bot
no flags Details
Patch (7.63 KB, patch)
2019-07-24 15:25 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.
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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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>