Bug 198630 – Three checks are missing in Proxy internal methods

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Shvayka]

Already implemented in Firefox Nightly.

ECMA262: https://github.com/tc39/ecma262/pull/666
Test262:
  https://test262.report/browse/built-ins/Proxy/defineProperty/targetdesc-not-configurable-writable-desc-not-writable.js
  https://test262.report/browse/built-ins/Proxy/getOwnPropertyDescriptor/resultdesc-is-not-configurable-not-writable-targetdesc-is-writable.js
  https://test262.report/browse/built-ins/Proxy/deleteProperty/targetdesc-is-configurable-target-is-not-extensible.js

[Alexey Shvayka]

Created attachment 371538 [details]
Patch

[Saam Barati]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alexey Shvayka]

Created attachment 371547 [details]
Patch

[Alexey Shvayka]

(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.

[Saam Barati]

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)

[Alexey Shvayka]

Created attachment 372107 [details]
Revert EXCEPTION_ASSERT change and add isExtensible test.

[EWS Watchlist]

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

[Saam Barati]

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?

[Alexey Shvayka]

Created attachment 372155 [details]
Patch

Add another isExtensible order test.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alexey Shvayka]

Comment on attachment 372155 [details]
Patch

Failed tests seem irrelevant.

[Alexey Shvayka]

Created attachment 374819 [details]
Patch

Add info about this feature shipping in other engines.

[WebKit Commit Bot]

Comment on attachment 374819 [details]
Patch

Clearing flags on attachment: 374819

Committed r247811: <https://trac.webkit.org/changeset/247811>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53524130>