40613 2010-06-15 00:00:38 -0700 The JSObjectSetProperty doesn't overwrite property flags. 2012-09-26 12:42:46 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED WONTFIX P2 Normal --- 40903 1 jedrzej.nowacki jedrzej.nowacki barraclough cmarcelo ggaren jedrzej.nowacki kent.hansen oliver oldest_to_newest 238265 0 jedrzej.nowacki 2010-06-15 00:00:38 -0700 It seems that the JSObjectSetProperty can't overwrite property flag. It simply ignores attributes argument while replacing property. 238267 1 58759 jedrzej.nowacki 2010-06-15 00:04:48 -0700 Created attachment 58759 Fix v1 241025 2 58759 oliver 2010-06-21 17:55:20 -0700 Comment on attachment 58759 Fix v1 This has bad behaviour in a number of ways -- the unconditional use of delete if the property exists will destroy the PIC and so hurt performance for cases where the attributes are not being changed. Additionally you get weird behaviour if have: proto = {prop: "foo"} o = Object.create(proto); and do: JSObjectSetProperty(o, "prop", ..., ...); you will delete "prop" from proto, and then add "prop" to o. Clearly this is less than ideal. :-( --Oliver 241184 3 kent.hansen 2010-06-22 01:39:31 -0700 (In reply to comment #2) > (From update of attachment 58759 [details]) > This has bad behaviour in a number of ways -- the unconditional use of delete if the property exists will destroy the PIC and so hurt performance for cases where the attributes are not being changed. > > Additionally you get weird behaviour if have: > > proto = {prop: "foo"} > o = Object.create(proto); > > and do: > JSObjectSetProperty(o, "prop", ..., ...); > > you will delete "prop" from proto, and then add "prop" to o. Clearly this is less than ideal. The hasProperty() call seems weird. Just because the object has some property in the prototype chain, the attributes you specify will be ignored when you create a property of the same name on your object? Would calling hasOwnProperty() be sufficient? Better, by calling getOwnPropertyDescriptor() one can get the attributes, and only delete the property if they differ. Oh, looks like JSObject::defineOwnProperty() already has such logic. ;) Still seems like a work-around, though. How about fixing JSObject::putWithAttributes() so it takes care of updating the attributes of an existing entry in case the property already exists (just like it takes care of updating the value)? Then unconditionally call putWithAttributes() from JSObjectSetProperty() when attributes != 0. (Which means there's no way to remove all attributes without recreating the property; oh well.) 576653 4 barraclough 2012-03-12 15:58:40 -0700 Sigh - the problem here is that it's not really clear what JSObjectSetProperty should be doing. Does it correspond to the spec's [[Put]] functionality? [[DefineOwnProperty]]? - or a mix of the two? Given the interface we have, if the property already exists on the object being put to, it probably does make sense to make this call defineOwnProperty so the attributes are updated accordingly. 728877 5 barraclough 2012-09-26 12:42:46 -0700 I don't think we're going to change this interface right now. The API behaviour is if ([[HasProperty]]) [[Put]] else [[DefineOwnProperty]] It might be nice to separate put & defineOwnProperty traps in the API, but JSObjectSetProperty's behaviour isn't going to change now. 58759 2010-06-15 00:04:48 -0700 2010-06-21 17:55:20 -0700 Fix v1 prop.diff text/plain 3105 jedrzej.nowacki ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0FQSS9KU09iamVjdFJlZi5jcHAgYi9KYXZhU2Ny aXB0Q29yZS9BUEkvSlNPYmplY3RSZWYuY3BwCmluZGV4IDVlMDUzNmYuLmIwMTkwMTMgMTAwNjQ0 Ci0tLSBhL0phdmFTY3JpcHRDb3JlL0FQSS9KU09iamVjdFJlZi5jcHAKKysrIGIvSmF2YVNjcmlw dENvcmUvQVBJL0pTT2JqZWN0UmVmLmNwcApAQCAtMjczLDcgKzI3MywxMCBAQCB2b2lkIEpTT2Jq ZWN0U2V0UHJvcGVydHkoSlNDb250ZXh0UmVmIGN0eCwgSlNPYmplY3RSZWYgb2JqZWN0LCBKU1N0 cmluZ1JlZiBwcm9wZQogICAgIElkZW50aWZpZXIgbmFtZShwcm9wZXJ0eU5hbWUtPmlkZW50aWZp ZXIoJmV4ZWMtPmdsb2JhbERhdGEoKSkpOwogICAgIEpTVmFsdWUganNWYWx1ZSA9IHRvSlMoZXhl YywgdmFsdWUpOwogCi0gICAgaWYgKGF0dHJpYnV0ZXMgJiYgIWpzT2JqZWN0LT5oYXNQcm9wZXJ0 eShleGVjLCBuYW1lKSkKKyAgICBpZiAoanNPYmplY3QtPmhhc1Byb3BlcnR5KGV4ZWMsIG5hbWUp KQorICAgICAgICBqc09iamVjdC0+ZGVsZXRlUHJvcGVydHkoZXhlYywgbmFtZSk7CisKKyAgICBp ZiAoYXR0cmlidXRlcykKICAgICAgICAganNPYmplY3QtPnB1dFdpdGhBdHRyaWJ1dGVzKGV4ZWMs IG5hbWUsIGpzVmFsdWUsIGF0dHJpYnV0ZXMpOwogICAgIGVsc2UgewogICAgICAgICBQdXRQcm9w ZXJ0eVNsb3Qgc2xvdDsKZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0FQSS90ZXN0cy90ZXN0 YXBpLmMgYi9KYXZhU2NyaXB0Q29yZS9BUEkvdGVzdHMvdGVzdGFwaS5jCmluZGV4IDc4MGU5OTYu LjY1NjMzNjggMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL0FQSS90ZXN0cy90ZXN0YXBpLmMK KysrIGIvSmF2YVNjcmlwdENvcmUvQVBJL3Rlc3RzL3Rlc3RhcGkuYwpAQCAtMTI1Miw3ICsxMjUy LDEwIEBAIGludCBtYWluKGludCBhcmdjLCBjaGFyKiBhcmd2W10pCiAgICAgSlNTdHJpbmdSZWxl YXNlKHN0cmluZyk7CiAKICAgICBvID0gSlNPYmplY3RNYWtlKGNvbnRleHQsIE5VTEwsIE5VTEwp OworICAgIEpTU3RyaW5nUmVmIGpzUHJvcGVydHlOYW1lID0gSlNTdHJpbmdDcmVhdGVXaXRoVVRG OENTdHJpbmcoIlBPTkciKTsKICAgICBKU09iamVjdFNldFByb3BlcnR5KGNvbnRleHQsIG8sIGpz T25lSVN0cmluZywgSlNWYWx1ZU1ha2VOdW1iZXIoY29udGV4dCwgMSksIGtKU1Byb3BlcnR5QXR0 cmlidXRlTm9uZSwgTlVMTCk7CisgICAgSlNPYmplY3RTZXRQcm9wZXJ0eShjb250ZXh0LCBvLCBq c1Byb3BlcnR5TmFtZSwgIEpTVmFsdWVNYWtlTnVtYmVyKGNvbnRleHQsIDEpLCBrSlNQcm9wZXJ0 eUF0dHJpYnV0ZU5vbmUsIE5VTEwpOworICAgIEpTT2JqZWN0U2V0UHJvcGVydHkoY29udGV4dCwg bywganNQcm9wZXJ0eU5hbWUsICBKU1ZhbHVlTWFrZU51bWJlcihjb250ZXh0LCAxKSwga0pTUHJv cGVydHlBdHRyaWJ1dGVEb250RW51bSwgTlVMTCk7CiAgICAgSlNPYmplY3RTZXRQcm9wZXJ0eShj b250ZXh0LCBvLCBqc0NGSVN0cmluZywgIEpTVmFsdWVNYWtlTnVtYmVyKGNvbnRleHQsIDEpLCBr SlNQcm9wZXJ0eUF0dHJpYnV0ZURvbnRFbnVtLCBOVUxMKTsKICAgICBKU1Byb3BlcnR5TmFtZUFy cmF5UmVmIG5hbWVBcnJheSA9IEpTT2JqZWN0Q29weVByb3BlcnR5TmFtZXMoY29udGV4dCwgbyk7 CiAgICAgc2l6ZV90IGV4cGVjdGVkQ291bnQgPSBKU1Byb3BlcnR5TmFtZUFycmF5R2V0Q291bnQo bmFtZUFycmF5KTsKQEAgLTEyNjAsNyArMTI2Myw4IEBAIGludCBtYWluKGludCBhcmdjLCBjaGFy KiBhcmd2W10pCiAgICAgZm9yIChjb3VudCA9IDA7IGNvdW50IDwgZXhwZWN0ZWRDb3VudDsgKytj b3VudCkKICAgICAgICAgSlNQcm9wZXJ0eU5hbWVBcnJheUdldE5hbWVBdEluZGV4KG5hbWVBcnJh eSwgY291bnQpOwogICAgIEpTUHJvcGVydHlOYW1lQXJyYXlSZWxlYXNlKG5hbWVBcnJheSk7Ci0g ICAgQVNTRVJUKGNvdW50ID09IDEpOyAvLyBqc0NGU3RyaW5nIHNob3VsZCBub3QgYmUgZW51bWVy YXRlZAorICAgIEpTU3RyaW5nUmVsZWFzZShqc1Byb3BlcnR5TmFtZSk7CisgICAgYXNzZXJ0VHJ1 ZShjb3VudCA9PSAxLCAianNDRlN0cmluZyBhbmQganNQcm9wZXJ0eU5hbWUgc2hvdWxkIG5vdCBi ZSBlbnVtZXJhdGVkLiIpOwogCiAgICAgSlNWYWx1ZVJlZiBhcmd1bWVudHNBcnJheVZhbHVlc1td ID0geyBKU1ZhbHVlTWFrZU51bWJlcihjb250ZXh0LCAxMCksIEpTVmFsdWVNYWtlTnVtYmVyKGNv bnRleHQsIDIwKSB9OwogICAgIG8gPSBKU09iamVjdE1ha2VBcnJheShjb250ZXh0LCBzaXplb2Yo YXJndW1lbnRzQXJyYXlWYWx1ZXMpIC8gc2l6ZW9mKEpTVmFsdWVSZWYpLCBhcmd1bWVudHNBcnJh eVZhbHVlcywgTlVMTCk7CmRpZmYgLS1naXQgYS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9K YXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKaW5kZXggOGQ1YjU5ZS4uYmIzMTJmZiAxMDA2NDQKLS0t IGEvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCisrKyBiL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxv ZwpAQCAtMSwzICsxLDIwIEBACisyMDEwLTA2LTE1ICBKZWRyemVqIE5vd2Fja2kgIDxqZWRyemVq Lm5vd2Fja2lAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIEZpeCB0aGUgSlNPYmplY3RTZXRQcm9wZXJ0eS4KKworICAgICAgICBPdmVy d3JpdGluZyBhbiBvYmplY3QgcHJvcGVydHkgd2l0aCB0aGUgSlNPYmplY3RTZXRQcm9wZXJ0eSBy ZXBsYWNlCisgICAgICAgIHByb3BlcnR5IGF0dHJpYnV0ZXMgKGZsYWdzKSB0b28uCisKKyAgICAg ICAgVGhlIEpTT2JqZWN0U2V0UHJvcGVydHkgZG9lc24ndCBvdmVyd3JpdGUgcHJvcGVydHkgZmxh Z3MuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00MDYx MworCisgICAgICAgICogQVBJL0pTT2JqZWN0UmVmLmNwcDoKKyAgICAgICAgKEpTT2JqZWN0U2V0 UHJvcGVydHkpOgorICAgICAgICAqIEFQSS90ZXN0cy90ZXN0YXBpLmM6CisgICAgICAgIChtYWlu KToKKwogMjAxMC0wNi0xMSAgRXJpYyBTZWlkZWwgIDxlcmljQHdlYmtpdC5vcmc+CiAKICAgICAg ICAgUmV2aWV3ZWQgYnkgQWRhbSBCYXJ0aC4K