Summary: | Three checks are missing in Proxy internal methods | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Alexey Shvayka
2019-06-06 16:46:04 PDT
Created attachment 371538 [details]
Patch
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 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 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
Created attachment 371547 [details]
Patch
(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 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) Created attachment 372107 [details]
Revert EXCEPTION_ASSERT change and add isExtensible test.
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 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? Created attachment 372155 [details]
Patch
Add another isExtensible order test.
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 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 on attachment 372155 [details]
Patch
Failed tests seem irrelevant.
Created attachment 374819 [details]
Patch
Add info about this feature shipping in other engines.
Comment on attachment 374819 [details] Patch Clearing flags on attachment: 374819 Committed r247811: <https://trac.webkit.org/changeset/247811> All reviewed patches have been landed. Closing bug. |