213041 2020-06-10 12:17:34 -0700 [JSC] JSCallbackObject::deleteProperty should redirect to Parent::deletePropertyByIndex if propertyName is index 2020-06-10 16:15:48 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ysuzuki ysuzuki darin ews-watchlist keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1661280 0 ysuzuki 2020-06-10 12:17:34 -0700 [JSC] JSCallbackObject::deleteProperty should redirect to Parent::deletePropertyByIndex if propertyName is index 1661282 1 401567 ysuzuki 2020-06-10 12:19:37 -0700 Created attachment 401567 Patch 1661283 2 ysuzuki 2020-06-10 12:19:39 -0700 <rdar://problem/64204300> 1661287 3 401567 darin 2020-06-10 12:28:04 -0700 Comment on attachment 401567 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401567&action=review > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:424 > + static_assert(std::is_final_v<JSCallbackObject<Parent>>, "Ensure no derived classes have custom deletePropertyByIndex implementation"); > + if (Optional<uint32_t> index = parseIndex(propertyName)) > + return Parent::deletePropertyByIndex(thisObject, globalObject, index.value()); > return Parent::deleteProperty(thisObject, globalObject, propertyName, slot); Seems like we have a small design problem here. Here we call parseIndex and determine something is not an index, but then call through to the parent without transmitting that information. Then, it also has to call parseIndex. Seems like there should be one function that doesn’t know if the property name is an index or not, another one that knows it’s an index, and a third that knows it’s not an index. Wouldn’t we like to make a design so that parsing to see if a property name is an index happens exactly once? 1661339 4 401567 ysuzuki 2020-06-10 13:45:17 -0700 Comment on attachment 401567 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401567&action=review >> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:424 >> return Parent::deleteProperty(thisObject, globalObject, propertyName, slot); > > Seems like we have a small design problem here. > > Here we call parseIndex and determine something is not an index, but then call through to the parent without transmitting that information. Then, it also has to call parseIndex. > > Seems like there should be one function that doesn’t know if the property name is an index or not, another one that knows it’s an index, and a third that knows it’s not an index. > > Wouldn’t we like to make a design so that parsing to see if a property name is an index happens exactly once? Yeah, agree. I think we eventually need to refactor these functions. Currently, deleteProperty accepts index and non-index. We should consider separating this into two functions. However, we need to be extra careful when doing that, otherwise, we accidentally introduce additional indirect calls which do not exist before. 1661361 5 401567 darin 2020-06-10 14:36:17 -0700 Comment on attachment 401567 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401567&action=review >>> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:424 >>> return Parent::deleteProperty(thisObject, globalObject, propertyName, slot); >> >> Seems like we have a small design problem here. >> >> Here we call parseIndex and determine something is not an index, but then call through to the parent without transmitting that information. Then, it also has to call parseIndex. >> >> Seems like there should be one function that doesn’t know if the property name is an index or not, another one that knows it’s an index, and a third that knows it’s not an index. >> >> Wouldn’t we like to make a design so that parsing to see if a property name is an index happens exactly once? > > Yeah, agree. I think we eventually need to refactor these functions. Currently, deleteProperty accepts index and non-index. We should consider separating this into two functions. > However, we need to be extra careful when doing that, otherwise, we accidentally introduce additional indirect calls which do not exist before. I agree with you, too. There’s danger of too many levels of function calls, other types of repeated work, and poor performance if this isn’t done right. It also offends my sensibilities to parse the same string twice. 1661399 6 ysuzuki 2020-06-10 15:35:20 -0700 In EWS, only imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-items-minimum-height-orthogonal-001.html is failing, but we know that this is failing without this patch. Landing. 1661418 7 ews-feeder 2020-06-10 16:15:47 -0700 Committed r262872: <https://trac.webkit.org/changeset/262872> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401567. 401567 2020-06-10 12:19:37 -0700 2020-06-10 16:15:48 -0700 Patch bug-213041-20200610121936.patch text/plain 4179 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjYyODI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA2 MjA2NmY4MGFlYzk3OWM0YTYxYjUzMTJmZjgzOTE4MmJlZTEzMzJlLi4yMzc3NjAzMzkwZDA3M2Nm Y2MzMjJjYTJhN2Y3ZWIxZDE5MTBlNzM3IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyNCBAQAorMjAyMC0wNi0xMCAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29t PgorCisgICAgICAgIFtKU0NdIEpTQ2FsbGJhY2tPYmplY3Q6OmRlbGV0ZVByb3BlcnR5IHNob3Vs ZCByZWRpcmVjdCB0byBQYXJlbnQ6OmRlbGV0ZVByb3BlcnR5QnlJbmRleCBpZiBwcm9wZXJ0eU5h bWUgaXMgaW5kZXgKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTIxMzA0MQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNjQyMDQzMDA+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2UgaGF2ZSBhbiBpbmZpbml0 ZSByZWN1cnNpb24gaGVyZS4KKworICAgICAgICAtPiBKU0NhbGxiYWNrT2JqZWN0OjpkZWxldGVQ cm9wZXJ0eUJ5SW5kZXgKKyAgICAgICAgICAgIC0+IEpTQ2VsbDo6ZGVsZXRlUHJvcGVydHkKKyAg ICAgICAgICAgICAgICAtPiBKU0NhbGxiYWNrT2JqZWN0OjpkZWxldGVQcm9wZXJ0eQorICAgICAg ICAgICAgICAgICAgICAtPiBKU09iamVjdDo6ZGVsZXRlUHJvcGVydHkKKyAgICAgICAgICAgICAg ICAgICAgICAgIC0+IEpTQ2FsbGJhY2tPYmplY3Q6OmRlbGV0ZVByb3BlcnR5QnlJbmRleAorCisg ICAgICAgIFdoZW4gcHJvcGVydHlOYW1lIGluIEpTQ2FsbGJhY2tPYmplY3Q6OmRlbGV0ZVByb3Bl cnR5IGlzIGFuIGluZGV4LCB3ZSBzaG91bGQgZ28gdG8gSlNPYmplY3Q6OmRlbGV0ZVByb3BlcnR5 QnlJbmRleCBpbnN0ZWFkIG9mIEpTT2JqZWN0OjpkZWxldGVQcm9wZXJ0eS4KKworICAgICAgICAq IEFQSS9KU0NhbGxiYWNrT2JqZWN0RnVuY3Rpb25zLmg6CisgICAgICAgIChKU0M6OkpTQ2FsbGJh Y2tPYmplY3Q8UGFyZW50Pjo6ZGVsZXRlUHJvcGVydHkpOgorCiAyMDIwLTA2LTA5ICBKb25hdGhh biBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNvbT4KIAogICAgICAgICBKYXZhU2NyaXB0Q29yZTog U3VwcG9ydCB0dk9TIGFuZCB3YXRjaE9TIGJ1aWxkcyB3aXRoIHRoZSBwdWJsaWMgU0RLCmRpZmYg LS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL0pTQ2FsbGJhY2tPYmplY3RGdW5jdGlv bnMuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvSlNDYWxsYmFja09iamVjdEZ1bmN0aW9u cy5oCmluZGV4IGFkMmQzOWUyYTdjYTc1NzUwNTQwN2U5MzcyNDgzMjQ1ZWY4NmVkNjIuLmExY2E0 MGVkNGNlZDBiNWE2MmQ0NzJlYjk4ZmY2ZTA2Yjk3Zjk1MGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS9BUEkvSlNDYWxsYmFja09iamVjdEZ1bmN0aW9ucy5oCisrKyBiL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9BUEkvSlNDYWxsYmFja09iamVjdEZ1bmN0aW9ucy5oCkBAIC00MTgs NiArNDE4LDkgQEAgYm9vbCBKU0NhbGxiYWNrT2JqZWN0PFBhcmVudD46OmRlbGV0ZVByb3BlcnR5 KEpTQ2VsbCogY2VsbCwgSlNHbG9iYWxPYmplY3QqIGdsb2IKICAgICAgICAgfQogICAgIH0KIAor ICAgIHN0YXRpY19hc3NlcnQoc3RkOjppc19maW5hbF92PEpTQ2FsbGJhY2tPYmplY3Q8UGFyZW50 Pj4sICJFbnN1cmUgbm8gZGVyaXZlZCBjbGFzc2VzIGhhdmUgY3VzdG9tIGRlbGV0ZVByb3BlcnR5 QnlJbmRleCBpbXBsZW1lbnRhdGlvbiIpOworICAgIGlmIChPcHRpb25hbDx1aW50MzJfdD4gaW5k ZXggPSBwYXJzZUluZGV4KHByb3BlcnR5TmFtZSkpCisgICAgICAgIHJldHVybiBQYXJlbnQ6OmRl bGV0ZVByb3BlcnR5QnlJbmRleCh0aGlzT2JqZWN0LCBnbG9iYWxPYmplY3QsIGluZGV4LnZhbHVl KCkpOwogICAgIHJldHVybiBQYXJlbnQ6OmRlbGV0ZVByb3BlcnR5KHRoaXNPYmplY3QsIGdsb2Jh bE9iamVjdCwgcHJvcGVydHlOYW1lLCBzbG90KTsKIH0KIApkaWZmIC0tZ2l0IGEvTGF5b3V0VGVz dHMvQ2hhbmdlTG9nIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCmluZGV4IGZlODcyOWZjYTk3NTQy NWFiMmMyMjg1OTBmYWMzMmEzZjAwY2QyNDQuLjBkMmJlYWZkODFhZjRiZTlmN2M2M2JiOTM3YTA4 MzU2OWMyYzljNzEgMTAwNjQ0Ci0tLSBhL0xheW91dFRlc3RzL0NoYW5nZUxvZworKysgYi9MYXlv dXRUZXN0cy9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAyMC0wNi0xMCAgWXVzdWtlIFN1 enVraSAgPHlzdXp1a2lAYXBwbGUuY29tPgorCisgICAgICAgIFtKU0NdIEpTQ2FsbGJhY2tPYmpl Y3Q6OmRlbGV0ZVByb3BlcnR5IHNob3VsZCByZWRpcmVjdCB0byBQYXJlbnQ6OmRlbGV0ZVByb3Bl cnR5QnlJbmRleCBpZiBwcm9wZXJ0eU5hbWUgaXMgaW5kZXgKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxMzA0MQorICAgICAgICA8cmRhcjovL3Byb2Js ZW0vNjQyMDQzMDA+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBqcy9kb20vY2FsbGJhY2tvYmplY3QtZGVsZXRlLXNob3VsZC1ub3QtaW5maW5pdGUt cmVjdXJzaW9uLWV4cGVjdGVkLnR4dDogQWRkZWQuCisgICAgICAgICoganMvZG9tL2NhbGxiYWNr b2JqZWN0LWRlbGV0ZS1zaG91bGQtbm90LWluZmluaXRlLXJlY3Vyc2lvbi5odG1sOiBBZGRlZC4K KwogMjAyMC0wNi0wOSAgRGllZ28gUGlubyBHYXJjaWEgIDxkcGlub0BpZ2FsaWEuY29tPgogCiAg ICAgICAgIFtHVEtdIEdhcmRlbmluZywgdXBkYXRlIHJlc3VsdHMgb2YgZmxha3kgdGltZW91dCBm YWlsdXJlcwpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvanMvZG9tL2NhbGxiYWNrb2JqZWN0LWRl bGV0ZS1zaG91bGQtbm90LWluZmluaXRlLXJlY3Vyc2lvbi1leHBlY3RlZC50eHQgYi9MYXlvdXRU ZXN0cy9qcy9kb20vY2FsbGJhY2tvYmplY3QtZGVsZXRlLXNob3VsZC1ub3QtaW5maW5pdGUtcmVj dXJzaW9uLWV4cGVjdGVkLnR4dApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi43ZWMwYjM5YjcyNjUwMWM0MDhkNWFjOWFl MjUxMTljYjU5ZWIyNjA5Ci0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvanMvZG9tL2Nh bGxiYWNrb2JqZWN0LWRlbGV0ZS1zaG91bGQtbm90LWluZmluaXRlLXJlY3Vyc2lvbi1leHBlY3Rl ZC50eHQKQEAgLTAsMCArMSwzIEBACisKK1BBU1MgSlNDYWxsYmFja09iamVjdDo6ZGVsZXRlUHJv cGVydHkgc2hvdWxkIG5vdCBjYXVzZSBpbmZpbml0ZSByZWN1cnNpb24gCisKZGlmZiAtLWdpdCBh L0xheW91dFRlc3RzL2pzL2RvbS9jYWxsYmFja29iamVjdC1kZWxldGUtc2hvdWxkLW5vdC1pbmZp bml0ZS1yZWN1cnNpb24uaHRtbCBiL0xheW91dFRlc3RzL2pzL2RvbS9jYWxsYmFja29iamVjdC1k ZWxldGUtc2hvdWxkLW5vdC1pbmZpbml0ZS1yZWN1cnNpb24uaHRtbApuZXcgZmlsZSBtb2RlIDEw MDY0NAppbmRleCAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi5lZDlm MGQzMTc3MTZlMmIwNGNkMjI1MGY2MGJhZTljOWZjOGY3YWM2Ci0tLSAvZGV2L251bGwKKysrIGIv TGF5b3V0VGVzdHMvanMvZG9tL2NhbGxiYWNrb2JqZWN0LWRlbGV0ZS1zaG91bGQtbm90LWluZmlu aXRlLXJlY3Vyc2lvbi5odG1sCkBAIC0wLDAgKzEsMTMgQEAKKzwhRE9DVFlQRSBIVE1MPgorPGh0 bWw+Cis8aGVhZD4KKyAgICA8c2NyaXB0IHNyYz0iLi4vLi4vcmVzb3VyY2VzL3Rlc3RoYXJuZXNz LmpzIj48L3NjcmlwdD4KKyAgICA8c2NyaXB0IHNyYz0iLi4vLi4vcmVzb3VyY2VzL3Rlc3RoYXJu ZXNzcmVwb3J0LmpzIj48L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5PgorPHNjcmlwdD4KK3Rlc3Qo KCkgPT4geworICAgIGRlbGV0ZSB0ZXN0UnVubmVyWzBdOworfSwgIkpTQ2FsbGJhY2tPYmplY3Q6 OmRlbGV0ZVByb3BlcnR5IHNob3VsZCBub3QgY2F1c2UgaW5maW5pdGUgcmVjdXJzaW9uIik7Cis8 L3NjcmlwdD4KKzwvYm9keT4K