Summary: | [[HasProperty]] result of Proxy in prototype chain is ignored | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Minor | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 203920 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Alexey Shvayka
2019-10-29 05:06:13 PDT
Created attachment 382177 [details]
Patch
Comment on attachment 382177 [details] Patch Attachment 382177 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13188171 New failing tests: stress/proxy-get-own-property.js.ftl-eager-no-cjit stress/proxy-get-prototype-of.js.no-cjit-validate-phases stress/for-in-side-effects.js.mini-mode stress/for-in-side-effects.js.dfg-eager stress/proxy-get-own-property.js.ftl-no-cjit-no-inline-validate stress/proxy-get-prototype-of.js.ftl-eager stress/proxy-get-own-property.js.no-cjit-validate-phases stress/proxy-get-own-property.js.dfg-eager stress/proxy-get-prototype-of.js.ftl-no-cjit-b3o0 stress/for-in-side-effects.js.bytecode-cache stress/proxy-get-own-property.js.no-cjit-collect-continuously stress/for-in-side-effects.js.dfg-eager-no-cjit-validate stress/proxy-get-own-property.js.ftl-eager stress/proxy-get-prototype-of.js.ftl-no-cjit-validate-sampling-profiler stress/proxy-get-prototype-of.js.ftl-eager-no-cjit stress/proxy-get-prototype-of.js.ftl-no-cjit-small-pool stress/for-in-side-effects.js.ftl-no-cjit-b3o0 stress/proxy-get-own-property.js.ftl-no-cjit-no-put-stack-validate stress/for-in-side-effects.js.no-ftl stress/proxy-get-prototype-of.js.eager-jettison-no-cjit stress/proxy-get-prototype-of.js.no-llint stress/proxy-get-prototype-of.js.bytecode-cache stress/proxy-get-own-property.js.no-ftl stress/proxy-get-own-property.js.default stress/proxy-get-own-property.js.no-llint stress/proxy-get-prototype-of.js.ftl-no-cjit-no-put-stack-validate stress/for-in-side-effects.js.ftl-no-cjit-validate-sampling-profiler stress/for-in-side-effects.js.ftl-no-cjit-no-put-stack-validate stress/proxy-get-prototype-of.js.ftl-no-cjit-no-inline-validate stress/proxy-get-own-property.js.bytecode-cache stress/for-in-side-effects.js.no-cjit-collect-continuously stress/proxy-get-prototype-of.js.dfg-eager-no-cjit-validate stress/for-in-side-effects.js.no-cjit-validate-phases stress/proxy-get-own-property.js.ftl-eager-no-cjit-b3o1 stress/for-in-side-effects.js.ftl-eager-no-cjit stress/proxy-get-prototype-of.js.ftl-eager-no-cjit-b3o1 stress/proxy-get-own-property.js.eager-jettison-no-cjit stress/for-in-side-effects.js.ftl-eager-no-cjit-b3o1 stress/proxy-get-prototype-of.js.mini-mode stress/proxy-get-prototype-of.js.no-ftl stress/for-in-side-effects.js.default ChakraCore.yaml/ChakraCore/test/es7/lookupgettersetter.js.default stress/for-in-side-effects.js.ftl-no-cjit-small-pool stress/proxy-get-own-property.js.dfg-eager-no-cjit-validate stress/proxy-get-prototype-of.js.dfg-eager stress/proxy-get-prototype-of.js.no-cjit-collect-continuously stress/for-in-side-effects.js.eager-jettison-no-cjit stress/for-in-side-effects.js.no-llint stress/proxy-get-own-property.js.ftl-no-cjit-b3o0 stress/proxy-get-own-property.js.ftl-no-cjit-validate-sampling-profiler stress/for-in-side-effects.js.ftl-eager stress/proxy-get-own-property.js.ftl-no-cjit-small-pool stress/proxy-get-own-property.js.mini-mode stress/for-in-side-effects.js.ftl-no-cjit-no-inline-validate stress/proxy-get-prototype-of.js.default Created attachment 382191 [details]
Patch
Add internalMethodType() check and adjust tests.
Comment on attachment 382191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382191&action=review r=me with a comment > Source/JavaScriptCore/runtime/JSObjectInlines.h:130 > + return hasSlot; Here and below, it seems like we could just return false instead? Created attachment 382491 [details]
Patch
Set reviewer and return false.
Do you have commit access set up now, Alexey? Otherwise I can cq+ if it's still in progress. (In reply to Ross Kirsling from comment #6) > Do you have commit access set up now, Alexey? Otherwise I can cq+ if it's > still in progress. Please do that, Ross. EWS failures seem irrelevant. I will be all set when Committer Agreement will be delivered to Apple and signed, which will take ~2 weeks. Comment on attachment 382491 [details] Patch Clearing flags on attachment: 382491 Committed r251940: <https://trac.webkit.org/changeset/251940> All reviewed patches have been landed. Closing bug. Hmm, looks like this patch caused 6 test262 failures: https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29/builds/6860 But they're all existing failures with updated results: test/built-ins/Array/prototype/reverse/length-exceeding-integer-limit-with-proxy.js test/built-ins/Array/prototype/splice/create-species-length-exceeding-integer-limit.js test/built-ins/Proxy/has/trap-is-undefined.js So I think we can commit an expectations.yaml update instead of rolling out. (In reply to Ross Kirsling from comment #11) > So I think we can commit an expectations.yaml update instead of rolling out. Er wait, no, test/built-ins/Proxy/has/trap-is-undefined.js is a totally new failure. I guess rollout is unavoidable. Re-opened since this is blocked by bug 203920 Created attachment 383041 [details]
Patch
Fix performDefaultHasProperty and adjust expectations.
Comment on attachment 383041 [details] Patch Clearing flags on attachment: 383041 Committed r252191: <https://trac.webkit.org/changeset/252191> All reviewed patches have been landed. Closing bug. Comment on attachment 383041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383041&action=review > Source/JavaScriptCore/runtime/JSObjectInlines.h:129 > + if (object->type() == ProxyObjectType && slot.internalMethodType() == PropertySlot::InternalMethodType::HasProperty) What about when it’s not in the prototype chain though? What if the proxy is the base? (In reply to Saam Barati from comment #17) > Comment on attachment 383041 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383041&action=review > > > Source/JavaScriptCore/runtime/JSObjectInlines.h:129 > > + if (object->type() == ProxyObjectType && slot.internalMethodType() == PropertySlot::InternalMethodType::HasProperty) > > What about when it’s not in the prototype chain though? What if the proxy is > the base? This case is handled by `JSObject* object = this;` on lines 121 and 152 (before loops), isn't it? |